Skip to content

Fix a severe bug in the code when the settings change might run even in the PR (NOP) mode. #342

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Dec 8, 2022
Merged

Fix a severe bug in the code when the settings change might run even in the PR (NOP) mode. #342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a7ac8ad
Get the endpoint for delete and not call the endpoint
decyjphr Nov 26, 2022
431a0ef
In nop mode get the remove endpoint url and not call it
decyjphr Nov 26, 2022
b57e8fd
update semver version
decyjphr Nov 26, 2022
484532b
Update create-pre-release.yml
decyjphr Nov 26, 2022
89098e8
Update create-pre-release.yml
decyjphr Nov 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/create-pre-release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,15 @@ jobs:
docker run --env APP_ID=${{ secrets.APP_ID }} --env PRIVATE_KEY=${{ secrets.PRIVATE_KEY }} --env WEBHOOK_SECRET=${{ secrets.WEBHOOK_SECRET }} -d -p 3000:3000 yadhav/safe-settings:main-enterprise
sleep 5
curl http://localhost:3000
- run: echo "${{ github.ref }}"
- name: Tag a final release
id: prerelease
uses: actionsdesk/[email protected].9
uses: actionsdesk/[email protected].10
with:
bump: ${{ inputs.bump }}
prerelease: ${{ inputs.prerelease }}
prelabel: ${{ inputs.prelabel }}
commitish: ${{ github.head_ref }}
commitish: ${{ github.ref }}
- name: Push Docker Image
if: ${{ success() }}
uses: docker/build-push-action@v2
Expand Down
2 changes: 1 addition & 1 deletion lib/plugins/collaborators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ module.exports = class Collaborators extends Diffable {
} else {
if (this.nop) {
return Promise.resolve([
new NopCommand(this.constructor.name, this.repo, this.github.repos.removeCollaborator(
new NopCommand(this.constructor.name, this.repo, this.github.repos.removeCollaborator.endpoint(
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity: What is the method endpoint doing? I could not find it in the docs: https://github.com/octokit/plugin-rest-endpoint-methods.js/blob/master/docs/repos/removeCollaborator.md.

Copy link
Collaborator Author

@decyjphr decyjphr Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is a method in the request that returns the params and the url that would be called by that particular request method. There is an example here and the code is here

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a dumb idea: How about making NopCommand making responsible for calling .endpoint in case endpoint argument is not null? It would require to provide the endpoint arguments as a separate parameter but at least you would ensure that NopCommand will be the only one calling these methods.

At least I can imagine that this bug might be introduced in the future again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, I'll make this change separately since it will affect the code broadly and we will need to test it more thoroughly

Object.assign(this.repo, { username: existing.username })
), 'Remove Collaborator')
])
Expand Down
2 changes: 1 addition & 1 deletion lib/plugins/teams.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ module.exports = class Teams extends Diffable {
remove (existing) {
if (this.nop) {
return Promise.resolve([
new NopCommand(this.constructor.name, this.repo, this.github.request(
new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint(
`DELETE ${teamRepoEndpoint}`,
{ team_id: existing.id, ...this.repo, org: this.repo.owner }
), 'DELETE Team')
Expand Down