Skip to content

fix: allow deletion of PR branch - #16

Merged
aduh95 merged 6 commits into
nodejs:mainfrom
aduh95:delete-head-branch
Dec 12, 2024
Merged

fix: allow deletion of PR branch#16
aduh95 merged 6 commits into
nodejs:mainfrom
aduh95:delete-head-branch

Conversation

@aduh95

@aduh95 aduh95 commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

By using the push event instead of the pull_request event, we can no longer care about whether the head branch gets deleted or not.

Comment thread .github/workflows/process.yml Outdated
@aduh95
aduh95 marked this pull request as ready for review December 10, 2024 18:03
@aduh95

aduh95 commented Dec 10, 2024

Copy link
Copy Markdown
Contributor Author

@joyeecheung Should we merge this, and revert if it doesn't work?

@joyeecheung

Copy link
Copy Markdown
Member

I would still prefer to somehow forbid sending actions via direct push to main, because it can be easy to make a mistake (I sometimes use the GitHub UI to just add a post, and if I accidentally choose "commit to the main branch", I left it there, and I am glad that the action would ignore that hiccup and I can just force push to remove it).

@aduh95

aduh95 commented Dec 10, 2024

Copy link
Copy Markdown
Contributor Author

@aduh95

aduh95 commented Dec 10, 2024

Copy link
Copy Markdown
Contributor Author

Alright, let's not use push event then, but let's disable "Rebase and merge" and use to our advantage that the PR lands in a single commit so we don't need to use the PR branch, just the commit on main. Would that be more acceptable?

@joyeecheung joyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks. Have you tried it in a test account?

Comment thread automation.md Outdated
@joyeecheung

Copy link
Copy Markdown
Member

I tried it in https://github.com/TestOrgPleaseIgnore/bluesky/actions/runs/12286204740/job/34285856571 and it doesn't seem to work, as it needs to check out the main branch to push (and I think fetch-deps does need to be 0?)

@joyeecheung

Copy link
Copy Markdown
Member

Anyway, seeing #23 being deleted in the fork changed my mind, I am okay with using the push event now, though I think that still requires squashing the commits so it needs to be documented?

@joyeecheung

Copy link
Copy Markdown
Member

I've set up https://github.com/TestOrgPleaseIgnore/bluesky which is setup to work with https://bsky.app/profile/pixel-voyager.bsky.social (I feel that maybe we should rename the account to be "PRIMARY" or something so that both repo can use the same alias to test and reduce conflicts, but for now in that repo it needs to be "PIXEL") and added you as maintainer, you can use that repo to test it live.

Comment thread .github/workflows/process.yml Outdated
@aduh95

aduh95 commented Dec 11, 2024

Copy link
Copy Markdown
Contributor Author

I think fetch-deps does need to be 0?

No I don't think there's any reason it would be necessary, it's perfectly fine to push from a shallow clone.

Could you try again with the latest commit I pushed please? EDIT: I see that you gave me access to the test org, so I went ahead and tried https://github.com/TestOrgPleaseIgnore/bluesky/actions/runs/12287030932/job/34288299714, it correctly created TestOrgPleaseIgnore@036a277

Comment thread .github/workflows/process.yml Outdated
@aduh95
aduh95 merged commit f108ed7 into nodejs:main Dec 12, 2024
@aduh95
aduh95 deleted the delete-head-branch branch December 12, 2024 00:06
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