Closed zhimsel closed 1 year ago
@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?
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).
@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.
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
🤷🏻
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
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: https://github.com/mheap/github-action-required-labels/pull/54/commits/ede948400991cbdc3a7569c1db7cfd06cabead1c#diff-59e25b254be93038f106111be580b6fb54c6963b6c4e7ef744e58fb8f2b3e3a2R18
Unless I misunderstand the testing framework and those ALSO need to be set in the environment outside of the test.
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
Ah, that makes sense. Thanks!
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 jobGITHUB_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.