| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Adapt relevant functions by narrowing their types or skipping steps that would otherwise have been skipped as well
There was a problem hiding this comment.
Warning
The central aborted-configuration persistence and restoration path lacks direct unit coverage, with additional minor diagnostic and documentation issues.
4 open findings🧠 Review effort: Balanced
Sorry, something went wrong.
| * The file will only contain parts of {@link config} that are relevant to | ||
| * the `init-post` step in the workflow. | ||
| */ | ||
| export function saveAbortedInitConfig( |
There was a problem hiding this comment.
Some suggestions and comments, but this broadly looks good to me. I'm happy with testing the new functionality here using the PR check added by #4223.
Sorry, something went wrong.
| * to requiring only those. For example, in `ActionConfig<"codeqlCmd">` | ||
| * all properties except `codeqlCmd` are optional. | ||
| */ | ||
| export type ActionConfig< |
There was a problem hiding this comment.
Bikeshed: ConfigWith?
Also, we should be careful with properties that might be optional, e.g. buildMode can be undefined in a successful configuration.
Sorry, something went wrong.
There was a problem hiding this comment.
Some time ago we talked about how Config is an awful name for the type and that it's really more representing the state of the Action that we persist across steps. It's also ambiguous as to whether it refers to the Action, CLI, or something else. I thought that I would use this opportunity to move away from that and introduce a better name.
Sorry, something went wrong.
There was a problem hiding this comment.
My one comment about ActionConfig is that it's quite easy to confuse with ActionState, e.g. you mention "A new ActionState type is added" in the PR description. But I agree it's good to clarify that we're talking about Action configuration.
Sorry, something went wrong.
There was a problem hiding this comment.
Good point and I have fixed that in the PR description now. I can't really think of a better name right now, but open to suggestions (and happy to rename as a follow-up).
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This PR mainly contains cherry-picked changes from #4223 for easier review and since they are somewhat separate from the main goal there. In #4223, the goal is to persist and upload diagnostics that are created before the database is initialised. That is the case if, for example, we produce diagnostics and then realise that we should abort the analysis and shouldn't initialise a database.
One issue with making diagnostics from a failed init run available is that the init-post action does not produce or upload any debug artifacts (including the failed SARIF file that would contain the diagnostics) if no Config was written by init. However, init only writes the Config these days if it successfully runs to completion.
This PR addresses this by making the following changes:
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
Products:
Environments:
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist