Fetch configuration content from PR ref #8
Conversation
Fixes actions#6 When fetching the configuration content from the file specified by `configuration-path`, we should load the content from the ref of the PR. By default, `repos.getContents` will [use the repository's default branch][1] if no `ref` is specified. When first configuring this action, assuming the configuration file is added in the same PR as the action, the configuration file will not exist on the default branch. This causes an `HttpError: Not Found` to occur when attempting to load the configuration from the default branch. This commit updates the call to `getContents` to add the `ref` parameter, set to the `GITHUB_SHA` value from the context. This will cause the action to load the contents of the configuration file from the ref of the PR, rather than the default branch. --- This PR also updates the packages to use version 1.0.0 of all the toolkit action packages, as they were using local files before. I noticed that `src/main.ts` is not formatted according to the prettier rules. I'm happy to open another PR that applies prettier to the current source. I had to disable prettier during implementation of this commit in order to keep the changes relevant to this PR. [1]: https://developer.github.com/v3/repos/contents/#get-contents
|
Why doesn't the action access the configuration from the local workspace git |
There actually isn't a local checkout of the repository. This action only uses the event payload and the API to label the PR. |
| @@ -92,7 +92,8 @@ async function fetchContent( | |||
| const response = await client.repos.getContents({ | |||
There was a problem hiding this comment.
Do we maybe want to retry on the default branch if this 404's? The scenario I'm concerned about is:
- User X branches repo
- This action gets merged into master
- User X PRs, but is out of sync with master
No label would get applied at this point if they hadn't synced with master yet, even if they sync. I know this is kinda an edge case, but its most likely to crop up the first couple of times the action is run and isn't a great first experience.
There was a problem hiding this comment.
I'm not sure I'd agree with retrying on the default branch. My feeling is that workflows/actions should only run against the code in the ref/sha they are committed to. I think I'd be surprised if my branch used actions/configuration from a different branch.
I think the scenario you described is fairly unlikely and has an obvious remedy (rebase/merge master).
What do you think of shipping this as-is, and making the change if we get feedback from the community?
There was a problem hiding this comment.
I think the scenario you described is fairly unlikely and has an obvious remedy (rebase/merge master).
Yeah, you're right - got this one confused with another action and was thinking that it only got triggered on a PR being opened, that's not right though.
Lets ship, I don't think we need this - thanks for pushing back.
|
Published! |
Fixes #6
When fetching the configuration content from the file specified by
configuration-path, we should load the content from the ref of the PR.By default,
repos.getContentswill use the repository's default branch if norefis specified.When first configuring this action, assuming the configuration file is added in the same PR as the action, the configuration file will not exist on the default branch. This causes an
HttpError: Not Foundto occur when attempting to load the configuration from the default branch.This commit updates the call to
getContentsto add therefparameter, set to theGITHUB_SHAvalue from the context. This will cause the action to load the contents of the configuration file from the ref of the PR, rather than the default branch.This PR also updates the packages to use version 1.0.0 of all the toolkit action packages, as they were using local files before.
I noticed that
src/main.tsis not formatted according to the prettier rules. I'm happy to open another PR that applies prettier to the current source. I had to disable prettier during implementation of this commit in order to keep the changes relevant to this PR.