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

Introduce notify-on-force-push workflow #7322

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bmuenzenmeyer
Copy link
Collaborator

@bmuenzenmeyer bmuenzenmeyer commented Dec 7, 2024

Description

Adopting the workflow from nodejs/node - https://github.com/rtCamp/action-slack-notify - which will be useful as we add more users with write access to the repo as part of CODEOWNERS work, AND it introduces accountability for existing users.

Should anyone be able to force push at all? I think the answer remains yes, but I want more visibility to it.

Validation

Post-merge, force pushes will send a Slack message to #nodejs-website

Related Issues

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

@Copilot Copilot bot review requested due to automatic review settings December 7, 2024 21:47
@bmuenzenmeyer bmuenzenmeyer requested a review from a team as a code owner December 7, 2024 21:47
Copy link

vercel bot commented Dec 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Dec 7, 2024 9:47pm

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.

Before: <https://github.com/${{ github.repository }}/commit/${{ github.event.before }}|${{ github.event.before }}>
After: <https://github.com/${{ github.repository }}/commit/${{ github.event.after }}|${{ github.event.after }}>
SLACK_USERNAME: nodejs-bot
SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need help understanding how to generate this. Will ask in Slack

@AugustinMauroy
Copy link
Member

Should anyone be able to force push at all?

IMO, no, we have a wait-before-merge policy. In my opinion, merge force push should only be used when it's absolutely necessary to restore/fix something.

- name: Slack Notification
uses: rtCamp/action-slack-notify@c33737706dea87cd7784c687dadc9adf1be59990 # 2.3.2
env:
SLACK_COLOR: '#DE512A'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SLACK_COLOR: '#DE512A'
SLACK_COLOR: '#DE512A' # orange

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I purposely kept it the same as the nodejs/node repo - but 🤷

Copy link
Member

Choose a reason for hiding this comment

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

yeah I just propose to explain which colors is behind this exa

@AugustinMauroy
Copy link
Member

Also in the rest markdown documents there is no specific mention of #nodejs-website maybe we should do so.

@bmuenzenmeyer
Copy link
Collaborator Author

IMO, no, we have a wait-before-merge policy. In my opinion, merge force push should only be used when it's absolutely necessary to restore/fix something.

Correct - sorry if my language was not clear. I agree there are circumstances that make it necessary / expedient / useful to have force push.

@bmuenzenmeyer
Copy link
Collaborator Author

Also in the rest markdown documents there is no specific mention of #nodejs-website maybe we should do so.

If I am understanding your concern, the webhook secret has the channel baked in to it, that's why you don't see it on the action config at all.

@bmuenzenmeyer bmuenzenmeyer marked this pull request as draft December 8, 2024 14:42
@bmuenzenmeyer
Copy link
Collaborator Author

switching this to DRAFT to make it clear the webhook secret is NOT yet in place

@AugustinMauroy
Copy link
Member

If I am understanding your concern, the webhook secret has the channel baked in to it, that's why you don't see it on the action config at all.

Oops, I didn't make myself clear, I was talking about adding a reference to the slack channel in the documentation to explain that part of the discussion takes place there. For example on the readme or contributing.md

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