-
-
Notifications
You must be signed in to change notification settings - Fork 53
feat: add --validate-no-metadata flag
#125
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
base: main
Are you sure you want to change the base?
Conversation
Useful to ensure the commit message doesn't include metadata that should be added by automation (PR-URL and reviewers).
|
No objection on the flag, but I'm not sure we will be able to add it to nodejs/node.
|
|
We don't land backports with the CQ, so I don't think that would be problematic. For cherry-picks, I don't think the commit would contain |
|
It does. At least for V8 patches, I always keep the metadata for the first time a commit landed. For example: nodejs/node@cc36db7 |
|
I see! So if we started to use that flag in Node.js, we would need to land those PRs manually. Would that disrupt your process? |
|
I could land them manually. |
|
I guess it would actually not apply for those, since the GHA commit linter is only validating the first commit message, and those floating patches are never first IIRC. |
They are on, e.g. nodejs/node#53997. |
I added a test with the commit on the PR you linked, it still passes with the new flag. |
We also add PR-URL to the release commits.. For most releases, the release commit won't be the first commit in the PR, but it will be for the LTS transition release. While we do not use the CQ on releases, it would be good to not have the commit linter fail for them. |
Sure but I don't think it's an objection to adding the flag, it sounds to me that's something to handle on nodejs/node side – i.e. the PR making use of this flag should make sure it's not applied to release proposals. |
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.
Changes LGTM.
I think it'll be confusing that we'll have both --no-validate-metadata and --validate-no-metadata but I don't have a suggestion for an alternative name for the new option.
Come to think of it, that means the negation of the new option will be |
|
Thinking more about this, other options would be to:
If we do the latter, we can probably rename the flag to |
Useful to ensure the commit message doesn't include metadata that should be added by automation (PR-URL and reviewers). In particular, we could use that in the commit linter in nodejs/node to report as invalid commits that include a
PR-URLorReviewed-bytrailers so the CQ refuse to land such commit as having those manually entered is error prone.