Warn when workflow analyzes the same language twice - #1901
henrymercer merged 5 commits into
Conversation
Sorry, something went wrong.
adityasharad
left a comment
There was a problem hiding this comment.
Good idea. A few minor questions.
Sorry, something went wrong.
| "starting", | ||
| startedAt, | ||
| await checkDiskUsage(logger), | ||
| workflowErrors, |
There was a problem hiding this comment.
This change means all workflow errors will be reported as Actions errors, but not part of the starting status report, right?
Sorry, something went wrong.
There was a problem hiding this comment.
That's right. This improves consistency since we're using the error and cause fields of the status report to indicate errors, rather than sometimes errors and sometimes warnings. I think if we want to collect workflow validation warnings, it would be more appropriate to add a new field.
Sorry, something went wrong.
| const matrixLanguagesByExtractor: { | ||
| [extractorName: string]: string[]; | ||
| } = {}; | ||
| for (const language of matrixLanguages) { | ||
| const extractorName = aliases[language] || language; | ||
| if (!matrixLanguagesByExtractor[extractorName]) { | ||
| matrixLanguagesByExtractor[extractorName] = []; | ||
| } | ||
| matrixLanguagesByExtractor[extractorName].push(language); | ||
| } |
There was a problem hiding this comment.
I think this could usefully be pulled out into a groupLanguagesByExtractorName helper.
Sorry, something went wrong.
| message: | ||
| `CodeQL language '${extractor}' is referenced by more than one entry in the ` + | ||
| `'language' matrix parameter for job '${jobName}'. This may result in duplicate alerts. ` + | ||
| `Please edit the 'language' matrix parameter to keep only one of the following: ${languages | ||
| .map((language) => `'${language}'`) | ||
| .join(", ")}.`, |
There was a problem hiding this comment.
Perhaps add a phrase like "these language names are considered identical by CodeQL". Do we want to recommend they use the combined alias key in this situation?
Sorry, something went wrong.
There was a problem hiding this comment.
The tricky thing is that the CLI doesn't really have a notion of the combined alias as the preferred name, since to reduce risk we did not change the names of the CodeQL extractors. I think we're going to hit this warning rarely enough that it's not worth hardcoding the language-specific information into the Action, but I don't feel strongly about this — feel free to convince me otherwise!
Sorry, something went wrong.
There was a problem hiding this comment.
Makes sense. We can introduce later a concept of "recommended name" if we want to do this, agree it's not urgent.
Sorry, something went wrong.
| t.deepEqual(...errorCodes(errors, [WorkflowErrors.CheckoutWrongHead])); | ||
| }); | ||
|
|
||
| test("getWorkflowErrors() for workflow with language name and its alias", async (t) => { |
There was a problem hiding this comment.
Just to be sure, have we got a test for two unrelated languages?
Sorry, something went wrong.
There was a problem hiding this comment.
Good idea, let's add one.
Sorry, something went wrong.
adityasharad
left a comment
There was a problem hiding this comment.
Optionally consider a changenote.
Sorry, something went wrong.
With the introduction of new aliases for C/C++, Java/Kotlin, and JavaScript/TypeScript, we want to avoid customers analyzing the same language more than once. The most common case of this will likely be customers who have started with a starter workflow and have added a duplicate language to the language matrix parameter.
This PR introduces workflow validation for this common case, so that customers who are analyzing the same language twice will get an Actions warning when they execute that workflow.
Merge / deployment checklist