Skip to content

Conversation

robertbrignull
Copy link
Contributor

@robertbrignull robertbrignull commented Apr 30, 2020

This changes the action to start uploading the analysis_key parameter but leaves the analysis_name unmodified.

The dotcom part of this migration is done so it should be ready to accept this parameter. @marcogario is this change good to make now?

@mveytsman would you be happy to review this as you reviewed the previous version of this PR?

Merge / deployment checklist

Copy link
Contributor

@marcogario marcogario left a comment

Choose a reason for hiding this comment

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

Yes, let's go ahead.

src/util.ts Outdated
const workflowPath = await getWorkflowPath();
const jobName = getRequiredEnvParam('GITHUB_JOB');

analysisKey = workflowPath + ' - ' + jobName;
Copy link
Contributor

Choose a reason for hiding this comment

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

@rneatherway had a suggestion to use a character that's illegal in the job name for the delimiter, and I agree. Unfortunately I can't find a reference for what characters are illegal, I think everything might be allowed.

Maybe we should still pick a delimiter that's less likely to be found in a path or job name, such as #?

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 point. I just forgot about this change.

Since this key is meant to be internal and we'll use the name for display purposes, it shouldn't matter if this is a little bit ugly. So we could use : or # and then URL-encode (or some other encoding mechanism) the other bits.

Does that sound reasonable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've gone with this for now, and I can obviously change it if there's a better option.

I think URL encoding has a positive trait in that it's 1-1, so there's no risk of two paths / job names accidentally merging to become the same key.

Copy link
Contributor

Choose a reason for hiding this comment

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

Job names must start with a letter or '' and contain only alphanumeric characters, '-', or '', see the error I got here: https://github.com/github/codeql-action-retired/actions/runs/99083794

It was good changing from - to :, but I think the URL encoding is not necessary, there is no risk of two paths / job names accidentally merging.

Besides that it looks good, I'll approve it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks for checking that. In that case I agree the URL encoding is unnecessary. I'll remove that and then I think this is good to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Daverlo, were you only suggesting removing the encoding for the job name, or both? The other thing the encoding was doing is making the : separator more clear. The workflow path can contain this character: e.g. https://github.com/Anthophila/test-electron/actions/runs/101574307/workflow

If the job name cannot contain a : character then using it as a separator should still be good enough for our purposes. You can recover the two parts by splitting on the last instance of :. This would still be possible to make ambiguous if someone makes the upload manually, but this is unlikely and we're not going for full separability of these two pieces of data and if we were we'd use two separate fields in the upload.

So I think removing the encoding will be fine. There will be no chance of clashes, and it's highly likely we can recover the separate workflow paths and job names in the future if we decide to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although also good to note that if you workflow path contains a : then the checkout fails on windows. I expect it would work on linux but the while workflow got aborted. I'm not interested enough to try it with just linux. The actions started executing and that's good enough for me.

@robertbrignull robertbrignull assigned Daverlo and unassigned mveytsman May 11, 2020
@robertbrignull robertbrignull merged commit c0d9de1 into master May 11, 2020
@robertbrignull robertbrignull deleted the analysisName branch May 11, 2020 16:50
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.

4 participants