In case you haven’t followed the issue from this official forum thread, there is currently an effort to ensure that the upstream Open edX PRs and their commits better describe their content, and doesn’t contain private references. So it’s worth reading Ed’s PR about this, to ensure we follow the practices described there - it helps to convince other developers to do something, if we already do it consistently ourselves. Thank you for the help!
8 Likes
Thanks for sharing and and highlighting Ed’s PR.
It is a common practice in our team, to add our internal jira ticket number in the subject line of upstream PRs, and although Ed’s PR is specifically targeted toward commit messages, should we move away from including our internal jira reference from PR subject lines as well ?
@antoviaque Thank you for sharing this with us. 
As far as I can tell, the main focus is on the commit messages, and even though there are brief mentions of PRs, I wasn’t able to find arguments or discussion behind it and the PR only makes changes to enforce the commit messages, so I think we should not change the workflow.
Imho, I agree with the reasoning behind enforcing such rule for commits, but I find it hard to understand why such rule would be needed for PRs (though I’m open to change my mind), other than perhaps removing ticket ids from the title of the PRs, since IIRC, the title of the PR goes into the commit message of the merge commit, thus the ticket id to the private JIRA ends up in the commit logs. I can think of many ways how those references in the PRs are useful to us (despite not being useful to others), and can’t think of how they are harmful to the rest of the community.
1 Like
The change is meant to apply to commits yes, though given the goal it would probably make sense to also remove our internal references from PR titles too (and leave them in PR bodies, with something similar to the ‘Private-ref:’ tag allowed for commits. The change from this PR is quite emphatic that commits “In no case should the subject contain a reference to an external system that is not accessible by all members of the Open edX community.”.
Btw, the PR has been merged, so this has now been enacted.
Also, it could make sense to do a PR against our own handbook to check details like this, and see precisely what it requires us to change or update in our processes. And then to get the review and 
of everyone in the team who will have to implement it – ie all developers.
Who would like to take a task to work on this? @braden would you be ok with being a main reviewer on it?
@antoviaque Sure. I’ll schedule a ticket for the upcoming sprint.
Edit: Created BB-6536
1 Like
Yep, I’ve added myself as the reviewer.
1 Like
Hello @team,
I have created public#465 to tweak our PR/commit process, to make sure we adhere to the changes discussed above.
Please do take a look, and leave any review comments/suggestions that you may have.
3 Likes