Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn when workflow analyzes the same language twice #1901

Merged
merged 5 commits into from
Sep 25, 2023

Conversation

henrymercer
Copy link
Contributor

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

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@henrymercer henrymercer requested a review from a team as a code owner September 22, 2023 14:11
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. A few minor questions.

if (
!(await sendStatusReport(
await createStatusReportBase(
"init",
"starting",
startedAt,
await checkDiskUsage(logger),
workflowErrors,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change means all workflow errors will be reported as Actions errors, but not part of the starting status report, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/workflow.ts Outdated
Comment on lines 128 to 137
const matrixLanguagesByExtractor: {
[extractorName: string]: string[];
} = {};
for (const language of matrixLanguages) {
const extractorName = aliases[language] || language;
if (!matrixLanguagesByExtractor[extractorName]) {
matrixLanguagesByExtractor[extractorName] = [];
}
matrixLanguagesByExtractor[extractorName].push(language);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could usefully be pulled out into a groupLanguagesByExtractorName helper.

Comment on lines +145 to +150
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(", ")}.`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. We can introduce later a concept of "recommended name" if we want to do this, agree it's not urgent.


t.deepEqual(...errorCodes(errors, [WorkflowErrors.CheckoutWrongHead]));
});

test("getWorkflowErrors() for workflow with language name and its alias", async (t) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure, have we got a test for two unrelated languages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, let's add one.

adityasharad
adityasharad previously approved these changes Sep 22, 2023
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optionally consider a changenote.

@henrymercer henrymercer merged commit 650a85e into main Sep 25, 2023
353 checks passed
@henrymercer henrymercer deleted the henrymercer/check-for-duplicated-languages branch September 25, 2023 13:25
@github-actions github-actions bot mentioned this pull request Sep 27, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants