Add category input by Daverlo · Pull Request #463 · github/codeql-action · GitHub

Follow up to #446

This PR introduces a new action input for both the analyze and upload actions for further configurability. The category is only applied as a substitute for the default analysis_key/environment, and it won't be applied if the sarif already contains an automation id.

Merge / deployment checklist

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

Upcoming versions of codeql database analyze will support the --category flag directly and put it into the SARIF. Should we look for that mechanism when it is supported? cc @aeisenberg

The cli v2.5.3 will automatically include an automationDetails.id value. It will be in the form specified in my comment. The cli will also accept a --sarif-category parameter that allows users to override the value with any string (though, the / is always appended if it's not there).

Even though v2.5.3 will support the category, sarif produced from other tools may not have a category. Also, it will be necessary to support older versions of the cli (or at least not crash on them).

I think, by default, the action should not be overriding the automationDetails.id value if one already exists, though there probably should be a way to force a custom id in there if requested.

Is this format specifically required? Currently, the cli will produce: javascript/linux/ (ie- <language>/<os>/) if the category is not specifically overriden.

No specific format is required, that's just a test example. It is based on the default format we decided for code scanning in #446, but we support any* string.

*As long as it complies with the sarif definition (the / at the end if there is no runId, etc)

Should this be updating the runner too? My understanding of this work was it's to allow parallel builds on 3rd party CI/CD, which means people using the runner and the external /sarifs endpoint.

Should this be updating the runner too? My understanding of this work was it's to allow parallel builds on 3rd party CI/CD, which means people using the runner and the external /sarifs endpoint.

That makes sense, thanks!

v2.5.3 of the cli will populate this value automatically. Is this something you want to overwrite here?

Here is a suggestion on how to handle this:

If the user supplies a custom automation id, then always overwrite what already exists.
If the action is using a generated automation id, then only overwrite if the id does not exist in the sarif yet.
Also, I think you need to be careful about setting run.automationDetails to a new object. It's not happening yet, but in the future, we may be using other properties of this object.

I don't think we should overwrite the id, that's why I went with your second suggestion.

Overwriting the value is quite tricky, and we should be careful about it. As you said, if we replace the full automationDetails object, we lose the possibility of using other properties in the future. And we can't just replace the id, as the other fields are correlated with it: #446 (comment)

The format of the cli-generated automationId is something linux/javascript/. This is different from what you are generating here. Is this something we should be unifying? Will code scanning work better if we change the cli to using the format you have here?

Code scanning will accept any valid string, so that is not a problem. Also, I don't think the cli can generate the same id, as the one generated by the action uses stuff specific to it, like the workflow file path and job name.

On the other hand, If we don't make it match we are going to have a mismatch on the comparison. I'll check with @starcke what do we want to do. My suggestion would be that the cli gets an input like the one I'm adding here, and from the action we generate the right string and pass it to the cli.

Is this something we are likely to hit in the wild? If so, it would be nice if we could handle this case better.

Yes, that is something we have already hit. For the moment we can't do it better, since we have been discarding this information in code scanning since the beginning.

analysis_key can be undefined (according to its type). I don't think that case is handled here.

analysis_key should not be undefined now, I'll change it.

Can you also add some tests when one or both of analysis_key and environment are undefined?

The function parameter type does not permit null for the environment. Either the type specification is incorrect, or you don't need to check environment !== "null". But maybe a simpler solution would be:

Suggested change

      
              if (environment !== undefined && environment !== "null") {
          
              if (environment) {

This would also avoid an error on an empty string.

I did change this, but rolled back because it was indeed needed https://github.com/github/codeql-action/pull/463/checks?check_run_id=2508425922. I don't know why, but sometimes we get environment = "null"

Hmmm...that's very odd. OK. I guess we need to keep it.

I mentioned in the tests above in a comment that this could cause ambiguity in the automationID. Is this serious enough to need to handle?

If we don't handle it all the users that have this kind of data will get some stale tips

This is good as long as we are sure that users will never user the cli < 2.5.3. I think that's the case, but just want to verify.

I think that should be the case, as the default bundle version is already updated https://github.com/github/codeql-action/pull/472/files.

For enterprise I think it should be safe too, but I'm not completely sure.

@robertbrignull could you please confirm if this change would be safe?

Since the action knows what version of the bundle it wants to download, it should be fine to rely on the CLI being new enough.

On dotcom you do need to wait for the actions virtual environment to be updated. Looking at a recent actions run on the repo that didn't set tools: latest, CodeQL appears to be at 2.5.4 so that's fine.

On enterprise server, the action and bundle should if anything stay more in sync because the action always tries to download/use the right bundle. When the action is updated in an air-gapped instance using the sync tool it syncs the action and the bundle together.

Of course people can always pass the tools input either with the action or the runner, but here we have to rely on people using the correct bundle version for the action/runner version. So for instance if they update the runner then they update the bundle at the same time. Hopefully it'll be fine.

I've looked through the production code at least and it all looks fine to me. I'll leave it to @aeisenberg to give a final review as you've been following more closely than I have and have already reviewed everything.

Looks good. Thanks for working through this and discovering the incident.

👍🏼

I run a test and this worked, it populated the category on the sarif https://github.com/dsp-testing/daverlo-main-flow/actions/runs/816024707

I'm going to hold off this PR a bit more, as I needed to specify tools: latest in order to succeed, as the default was still picking up v2.5.2 20210421 https://github.com/dsp-testing/daverlo-main-flow/actions/runs/816011280

The tools are already updated so I'll proceed to merge this https://github.com/dsp-testing/daverlo-main-flow/actions/runs/828192431

Add category input

40fb1f3

aeisenberg reviewed Apr 28, 2021

View reviewed changes

Don't use getOptionalInput on the runner codepath

76f5ada

Add category option to runner

c6e734c

aeisenberg reviewed Apr 30, 2021

View reviewed changes

Daverlo added 4 commits May 3, 2021 19:36

Add actions-util.getAutomationID()

519d077

Forward category input to codeql cli

c93cbc9

Use actionsUtil.computeAutomationID on upload-lib

3b741b3

Use the category on the runner

aa53f64

aeisenberg reviewed May 3, 2021

View reviewed changes

Daverlo added 2 commits May 5, 2021 12:30

Address comments

cd7eedd

Merge branch 'main' into daverlo/categoryInput

a117668

robertbrignull reviewed May 5, 2021

View reviewed changes

Fix undefined environment

0c0bc0e

Daverlo force-pushed the daverlo/categoryInput branch from 7f70758 to 0c0bc0e Compare May 5, 2021 13:46

aeisenberg approved these changes May 5, 2021

View reviewed changes

Merge branch 'main' into daverlo/categoryInput

ea18d47

Daverlo merged commit cf682cf into main May 10, 2021

Daverlo deleted the daverlo/categoryInput branch May 10, 2021 13:35

github-actions Bot mentioned this pull request May 17, 2021

Merge main into v1 #504

Merged

+              function computeAutomationID(
+                analysis_key: string | undefined,
+                environment: string | undefined
+              ): string {

+                analysis_key: string | undefined,
+                environment: string | undefined
+              ): string {
+                let automationID = `${analysis_key}/`;

+                  ".github/workflows/codeql-analysis.yml:analyze",
+                  '{"number": 1, "object": {"language": "javascript"}}'
+                );
+                t.deepEqual(

+                    } else {
+                      // In code scanning we just handle the string values,
+                      // the rest get converted to the empty string
+                      automationID += `${entry[0]}:/`;

                 let expectedSarif =
-                  '{"runs":[{"automationDetails":{"id":".github/workflows/codeql-analysis.yml:analyze/language:javascript/os:linux/"}}]}';
+                  '{"runs":[{"automationDetails":{"id":"language:javascript/os:linux/"}}]}';

+                    run.automationDetails = {
+                      id: automationID,
+                    };

+                );
+                // check non string environment values
+                actualAutomationID = actionsutil.computeAutomationID(

	if (environment !== undefined && environment !== "null") {
	if (environment) {

+                    if (automationDetailsId !== undefined) {
+                      args.push("--sarif-category", automationDetailsId);
+                    }

Add category input by Daverlo · Pull Request #463 · github/codeql-action · GitHub

Add category input - #463

Add category input#463Daverlo merged 11 commits intomaingithub/codeql-action:mainfrom daverlo/categoryInputgithub/codeql-action:daverlo/categoryInputCopy head branch name to clipboard

Conversation

Daverlo commented Apr 28, 2021

Merge / deployment checklist

Uh oh!

adityasharad commented Apr 28, 2021

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbrignull commented Apr 29, 2021

Uh oh!

Daverlo commented Apr 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbrignull left a comment

Choose a reason for hiding this comment

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Daverlo commented May 6, 2021

Uh oh!

Daverlo commented May 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add category input#463
Daverlo merged 11 commits into
mainfrom
daverlo/categoryInput