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

feat: add --validate-no-metadata flag #125

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 2, 2024

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-URL or Reviewed-by trailers so the CQ refuse to land such commit as having those manually entered is error prone.

Useful to ensure the commit message doesn't include metadata that should
be added by automation (PR-URL and reviewers).
@targos
Copy link
Member

targos commented Sep 2, 2024

No objection on the flag, but I'm not sure we will be able to add it to nodejs/node.
There are valid commits with metadata:

  • backports
  • cherry-picks of floating patches (on V8 for example)

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 2, 2024

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 PR-URL or Reviewed-by trailers, would they? If not, then this flag won't cause a problem since it's only checking for those two.

@targos
Copy link
Member

targos commented Sep 2, 2024

It does. At least for V8 patches, I always keep the metadata for the first time a commit landed. For example: nodejs/node@cc36db7

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 2, 2024

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?

@targos
Copy link
Member

targos commented Sep 2, 2024

I could land them manually.

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 2, 2024

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.

@richardlau
Copy link
Member

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.

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 2, 2024

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.

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.

3 participants