-
Notifications
You must be signed in to change notification settings - Fork 335
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
release-process: explain how and when to merge master into release-next #1192
release-process: explain how and when to merge master into release-next #1192
Conversation
Signed-off-by: Maël Valais <mael@vls.dev>
When creating a documentation PR for an unreleased feature, you will need to | ||
fast-forward the `release-next` branch to `master` before asking for a review of | ||
the PR: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied from #1081 (comment):
I do not agree with this.
It may be desirable to bring release-next up to date before submitting your release-next PR,
but usually only if the new feature you are documenting might conflict with some changes which have meanwhile been merged into the live documentation.In any case, release-next doesn't have to be "fast-forwarded"...just merged....unless there's a way to create a PR which really does fast forward the commits from one branch to another. Is that possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I think I misunderstood the implications of not having release-next
up-to-date. Even in my case, I didn't have to update release-next
to match master
.
- I will rephrase to make it an optional step only needed if the changes you intend to make to the documentation relies on changes that were made recently to master and aren't in release-next.
- I will use the word "merge master into release-next" instead of "fast-forward release-next to master". I agree with you, it can't be fast-forwarding and merge commits are needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have addressed your comments. Please take another look when you have time.
✅ Deploy Preview for cert-manager-website ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Signed-off-by: Maël Valais <mael@vls.dev>
@wallrj I think this is ready to be merged. |
Signed-off-by: Maël Valais <mael@vls.dev>
@wallrj I have removed most of what I had written since I realized that it was already explained in the README. The only thing I kept (and moved to the README) is how to merge master into release-next, and why that may be needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @maelvls
/lgtm
It is necessary because some the merge commits have been written by the bot | ||
and do not have a DCO signoff. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's possible to configure the bot to use --signoff when merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inteon investigated this idea (message):
From what I found it is not possible to add DCO signoff in the commit message. However, I found that the DCO check should actually check if the committer is a bot and not complain in that case, this is not well-documented and needs further investigation. For now, I found that just merging in case all other checks are OK works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was specific for that generic autobumper tool.
I don't know what tool is used here.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maelvls, wallrj The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
These changes were extracted from #1081 (comment).