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

authors: add annaw2193 to authors #124145

Merged
merged 1 commit into from
May 15, 2024
Merged

Conversation

annaw2193
Copy link
Contributor

@annaw2193 annaw2193 commented May 14, 2024

Epic: None
Release note: None

@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented May 14, 2024

CLA assistant check
All committers have signed the CLA.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@annaw2193 annaw2193 marked this pull request as draft May 14, 2024 17:22
Copy link
Collaborator

@jlinder jlinder left a comment

Choose a reason for hiding this comment

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

The change is good. A few comments on the PR body and commit message.

  • The Release Note line should be last in the commit message. The parser for them is greedy and takes everything after it as part of the release note. Could you update the commit message to place it last? The Updating PRs during review page discusses how to do this.
  • Did one of the linters ask for the Release justification line in the PR description? That should only be needed for backport PRs.

Also, this PR looks ready to be moved out of Draft mode. Normally, we used draft mode here to indicate that a PR isn't ready for review yet.

Epic: None
Release note: None
@annaw2193
Copy link
Contributor Author

The change is good. A few comments on the PR body and commit message.

  • The Release Note line should be last in the commit message. The parser for them is greedy and takes everything after it as part of the release note. Could you update the commit message to place it last? The Updating PRs during review page discusses how to do this.
  • Did one of the linters ask for the Release justification line in the PR description? That should only be needed for backport PRs.

Also, this PR looks ready to be moved out of Draft mode. Normally, we used draft mode here to indicate that a PR isn't ready for review yet.

  • Sounds good, I will update the commit message. The ordering of the release note line was based on the document provided for the first PR, should I update this document with the Release note line last in the commit message instead to reflect this?

  • I think I added it because I wasn't sure if it was necessary or not, but no problem I can remove it and I will keep that in mind for future reference

@annaw2193 annaw2193 requested a review from jlinder May 14, 2024 18:36
@annaw2193 annaw2193 marked this pull request as ready for review May 14, 2024 18:36
@annaw2193 annaw2193 added the first-pr Use to mark the first PR sent by a contributor / team member. Reviewers should be mindful of this. label May 14, 2024
Copy link
Collaborator

@jlinder jlinder left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

The ordering of the release note line was based on the document provided for the first PR, should I update this document with the Release note line last in the commit message instead to reflect this?

Ah. I see. These guides are updateable. How about putting in a change suggestion on that doc to correct the ordering?

@annaw2193
Copy link
Contributor Author

LGTM 🎉

The ordering of the release note line was based on the document provided for the first PR, should I update this document with the Release note line last in the commit message instead to reflect this?

Ah. I see. These guides are updateable. How about putting in a change suggestion on that doc to correct the ordering?

sounds good! I've made some suggestions in the doc to reflect this

@annaw2193
Copy link
Contributor Author

bors r+

@craig craig bot merged commit f691d11 into cockroachdb:master May 15, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-pr Use to mark the first PR sent by a contributor / team member. Reviewers should be mindful of this.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants