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

contributing: advice from recent RTI learnings #100

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

Conversation

iximeow
Copy link

@iximeow iximeow commented Dec 13, 2024

observations i found useful in figuring out what i should do to get to RTI email materials. the contributing docs are clear enough on what goes in the email, but i had a harder time figuring out if what i was putting together had the right information.

generally i had a hard time figuring out what the human on the receiving end needs or will do, so i'm trying to be clearer about that without being prescriptive. in particular, understanding that better from talking with @citrus-it is what helped me understand the variance in RTIs in practice.

it seems like the patch file itself can be optional if the correct patch is reflected in Gerrit, that pbchk isn't necessary if it's empty, contributors provide Reviewed by: (no tooling on the Gerrit or integrator side). whatchanged can either be in-line in the email if it is tiny, or attached if it would be large. these are what the RTI is expected to look like, if it differs that's not necessarily a problem but may place additional burden on the core team, so.. don't do that lightly.

in particular i'd looked at a few prior RTIs and saw that generally patches on the mailing list are not the same commit as on Gerrit, and that the patch subject line is basically uninteresting to the RTI recipient. it seems like if i was going to outline what core members are seeking, it is:

  • if on Gerrit, patch on Gerrit with Change-ID: which will be removed
  • if via email, patch contents from email, patch description from whatchanged
  • mail_msg in either case that describes the build when integrating the patch

finally, if you're like me and hop between commits regularly, mail_msg will be bogus if you built the gate, built a different commit, then switched to your RTI commit and built that. so also a heads to folks that that will not be good for RTI!

an open question i have here is: if my change was reviewed on Gerrit, and i've amended that commit like

issue# title
Reviewed by: name <email>

Change-ID: I1234

then is there value submitting the patch in my RTI email? personally speaking, that risks me attaching an incorrect commit, or Gerrit having a hopefully-minor stale commit that was in the email. this depends on how the core team fetches patches for integration. https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-patch can a change as a patch in base64, so "patch optional if using Gerrit" would be nice as a formal policy.

@jclulow
Copy link
Member

jclulow commented Dec 17, 2024

an open question i have here is: if my change was reviewed on Gerrit
then is there value submitting the patch in my RTI email?

No, there is no value to doing that. I could have sworn I had written this in the instructions, but apparently I merely dreamt doing that.

@rmustacc
Copy link
Contributor

I agree that there's no point in including that. @iximeow would you mind taking a pass to remove mention of sending the patch? I could also do that if you prefer. Thanks for taking a swing at improving this.

@iximeow
Copy link
Author

iximeow commented Dec 19, 2024

ok, that's helpful! i've amended the wording so that we're only mentioning attaching a patch if for some reason your change wasn't on Gerrit in the first place

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants