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

Support running multiple instances without collision #54

Merged
merged 2 commits into from
Jun 8, 2023

Conversation

zhimsel
Copy link
Contributor

@zhimsel zhimsel commented May 31, 2023

This uses a combination of three Github-set env vars that, together, are
guaranteed to be unique within the scope of a repo and all its
workflows:

  • GITHUB_WORKFLOW: the name of the workflow (or file path, if no name is given)
  • GITHUB_JOB: the ID of the job
  • GITHUB_ACTION: the ID of the step (or a formatted name of the repo where the action resides, i.e. __mheap_github-action-required-labels)

Without this, running multiple instances of this action with comments
enabled would result in them clobbering each others' comments.

Testing

I've tested this in my own environment and it works as expected. Admittedly, testing was fairly narrow, but covered what I considered enough ground.

I've also modified the unit tests accordingly.

@mheap mheap force-pushed the support-multiple-matchtokens branch from 6c55dc9 to df979de Compare June 5, 2023 12:09
@mheap
Copy link
Owner

mheap commented Jun 5, 2023

@zhimsel Do you think specifying a suffix manually is the correct approach here? We could do something like md5(labels) and use that as the token to make it transparent to the end user. What do you think?

@zhimsel
Copy link
Contributor Author

zhimsel commented Jun 5, 2023

Hmm, I like the idea of the short-lived generated token instead of the user specified one.

However, I think a checksum of just the labels may not be enough. While unlikely, it's certainly possible to have multiple jobs configured to act on the same set of labels (resulting in the same checksum).

Hm, granted, that's probably "good enough" for almost all cases. Though I can't imagine a scenario on the same label set that aren't directly related (and thus comment conflict is likely not a problem).

Let me think on this. I'll push up a change (hopefully today).

@zhimsel zhimsel force-pushed the support-multiple-matchtokens branch from df979de to dba8fc2 Compare June 5, 2023 14:01
@zhimsel zhimsel marked this pull request as draft June 5, 2023 14:02
@zhimsel zhimsel force-pushed the support-multiple-matchtokens branch from dba8fc2 to ede9484 Compare June 5, 2023 16:07
@zhimsel
Copy link
Contributor Author

zhimsel commented Jun 5, 2023

@mheap On thinking about it, a checksum of any dynamic info (like the label set) would not work, as the key could change between runs.

I thought of a different solution: using the workflow name, job ID, and step ID. I just pushed those changes (and updated the description of this PR).

However, I cannot get the unit tests to work without actually setting the required env vars. The unit test pass with the following:

export GITHUB_WORKFLOW="demo-workflow"
export GITHUB_JOB="demo-job"
export GITHUB_ACTION="required-labels"
npm test

index.js doesn't seem to be using the mocked env vars, but instead using the real environment. The tests also pass if I hardcode the matchtoken in index.js to be the mocked value.

@zhimsel zhimsel marked this pull request as ready for review June 5, 2023 16:13
@zhimsel
Copy link
Contributor Author

zhimsel commented Jun 5, 2023

Oh, well, hm... that's weird. Unit tests seem to be passing in the GH actions in this repo: https://github.com/mheap/github-action-required-labels/actions/runs/5179362225/jobs/9332133674

However, it fails in my fork: https://github.com/zhimsel/github-action-required-labels/actions/runs/5179361993/jobs/9332132547

🤷🏻

@mheap
Copy link
Owner

mheap commented Jun 6, 2023

Ah, that'll be due to the use of pull_request_target which runs tests on main rather than your branch. That's my mistake.

The tests require environment variables to be set. You can see them at https://github.com/mheap/github-action-required-labels/blob/main/index.test.js#L16-L26

You can set per-test environment variables too https://github.com/mheap/github-action-required-labels/blob/main/index.test.js#L52-L55

@zhimsel
Copy link
Contributor Author

zhimsel commented Jun 6, 2023

The tests require environment variables to be set. You can see them at https://github.com/mheap/github-action-required-labels/blob/main/index.test.js#L16-L26

Yeah, I set the new one for all tests here: ede9484#diff-59e25b254be93038f106111be580b6fb54c6963b6c4e7ef744e58fb8f2b3e3a2R18

Unless I misunderstand the testing framework and those ALSO need to be set in the environment outside of the test.

This uses a combination of three Github-set env vars that, together, are
guaranteed to be unique within the scope of a repo and all its
workflows:

- GITHUB_WORKFLOW: the name of the workflow (or file path, if no name is given)
- GITHUB_JOB: the ID of the job
- GITHUB_ACTION: the ID of the step (or a formatted name of the repo
  where the action resides, i.e.
  `__mheap_github-action-required-labels`)

Without this, running multiple instances of this action with comments
enabled would result in them clobbering each others' comments.
@zhimsel zhimsel force-pushed the support-multiple-matchtokens branch from ede9484 to 8156efe Compare June 6, 2023 13:56
@mheap
Copy link
Owner

mheap commented Jun 8, 2023

mocked-env allows you to set the environment variables within the test. The issue here is that we were trying to read the environment variables outside of action() (when the file is loaded) which is before we define the values in the test case. My most recent commit makes the tests pass again

@mheap mheap merged commit 418d9eb into mheap:main Jun 8, 2023
@zhimsel zhimsel deleted the support-multiple-matchtokens branch June 8, 2023 22:07
@zhimsel
Copy link
Contributor Author

zhimsel commented Jun 8, 2023

Ah, that makes sense. Thanks!

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.

2 participants