Skip to content

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.

@richardlau
Copy link
Member

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.

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.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 22, 2025

We also add PR-URL to the release commits.

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.

Copy link
Member

@richardlau richardlau left a 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.

@richardlau
Copy link
Member

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 --no-validate-no-metadata 😆.
OTOH since --validate-metadata is true, are there valid scenarios to run --no-validate-metadata where the commit has metadata (i.e. could this be the negation of the existing option?)? 🤔

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 22, 2025

Thinking more about this, other options would be to:

  • filter out trailers such as Reviewed by and PR-URL in NCU – so having wrong trailers in the PR has no impact in the commit that actually lands, and having them reported by core-validate-commit would not make sense.
  • only report as error if the trailer value is incorrect (i.e. it's fine to have a PR-URL trailer as long as it matches the actual PR-URL) – not sure we could do that for Reviewed-by though, we would still have to chose whether to ignore, filter, or report.

If we do the latter, we can probably rename the flag to --validate-metadata-for-unmerged-pr or something like that

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