Discourage PR authors from leaving important context behind private links

kdmccormick commented 2 years ago

Context

Private links (to Jira tickets, wiki pages, etc) in pull requests and commit messages make it harder for community members to:

diagnose bugs months later during the community release process (BTR WG, et al)
understand code changes years down the road (DEPR WG, et al)
and so on.

This has always been a problem, and it's not unique to Open edX. It might end up being exacerbated, though, by edX moving to a private Jira instance, 2u-internal.atlassian.net.

Goal

Encourage Open edX community members to include ALL relevant context in their PR description and/or commit message, with all critical supplementary links being public. This may involve a multi-pronged, ongoing approach.

Non-goal: Disallowing any private links in PR descriptions at all. Back-linking to private systems can be really useful to individual companies. Also, if we were to disallow this, there are always workarounds (eg, including the ticket number but not including it as a link)

Ideas

Outreach

Reach out to senior engineers, asking them to nudge junior engineers in their team/org about this. We have done this a bit already via edX's Arch Hour.

Surface the problem better

It was mentioned that junior engineers may not understand why changes they are making would be impactful to other engineers in the community. ie, "it's a simple change in a business-specific part of the code, why do I need to explain myself?". Of course, simple business-specific changes are often of interest to folks who are putting together releases or removing/refactoring years down the line.

Idea: Whenever someone comes across a PR with info they need hidden behind a private link, they should do X, where X could be:

leaving a comment explaining that the information is hidden behind a private link, for the author to see
adding a certain label indicating the issue
complaining about it in a specific channel

Pros: Would help PR authors understand the importance of the issue, increasing buy-in. Cons: Put the burden on community members instead of on PR authors.

PR templates

Change the PR template(s) to include a reminder about this issue.

Pros: Dead simple to implement Cons: Often are ignored, relegated to being white noise

Documentation

Document the best practices for opening PRs, including the requirement that context shouldn't be behind private links. Could be an OEP, or could be a simple documentation page.

Pros: Linking someone to a pre-agreed-upon document is easy. It takes some of the burden away from the reviewers, because instead of having to justify the cause of keeping context public, they can link to a place where it's already justified.

Cons: Someone most likely needs to actively link to the documentation in order for the PR author to notice it (unless we, say, made it part of the engineer onboarding process).

Aside: I believe an early edX version of this doc page exists somewhere. If anyone finds it, link it here!

A nag bot

Could be a:

GH actions check
that does not block the PR merge
but leaves a friendly message reminding folks when a private links is posted.

Pros:

simple
automatic

Cons:

it'd nag people who dutifully copy in all their context but also leave a private link
- maybe we could add a heuristic, like PR description length

nedbat commented 2 years ago

Is the goal here to get people to stop using 2u-internal links? Or for them to mark them appropriately? Or some other thing?

kdmccormick commented 2 years ago

@nedbat I updated the issue description with notes from Arch Hour.

sarina commented 2 years ago

maybe we could add a heuristic, like PR description length

This is hard because a lot of PRs include a commented out PR template, and if you entirely leave that template and just drop a link to Jira, it would still be long. I'm curious why a short "Hi! I noticed you included a link to a Jira tracker that may be private. Please make sure you've also included the ticket details in your PR description so those without access to your tracker may be able to understand your change" would be super annoying - you can ignore the message if you've already done this.

kdmccormick commented 2 years ago

Why would be annoying? It is hard for me to justify why something is or isn't annoying 🤷🏻 I will try to explain my thinking but it'll ultimately come down to how well my own preferences map onto others'.

At edX, it drove me bonkers when that Jira bot automatically said "Someone has left a comment on the linked CR ticket, please take a look!" every time I commented on a CR ticket. It made bug triage, which already wasn't very fun, a little less fun. Instead of being rewarded every time I communicated with someone on a CR, I was penalized by this stupid bot emailing me to inform me that someone (me) had left a comment. Eventually, I filtered these emails out, instinctively deleting them. There may have been some cases where I wasn't following a linked CR ticket that I should have been, so the bot's notification would have been useful. But, I didn't see those, because I had completely tuned out anything the bot was saying since 90% of it was noise. I am sure that whoever set up that Jira bot didn't have the intention of me being annoyed and ignoring their bot entirely, but it's what ended up happening.

Similarly, if a bot says "Hey, that's a private link" on every PR with private links no matter how thorough vs barebones its description is, I would expect developers to tune it out over time. Generalizing, I think the more often we nag people, the less attention they'll pay to that nag, and I also think this can have spillover effects into how much attention folks pay to Open edX community engineering standards as a whole. In other words, I think there's a "nag budget" that we need to be cognizant of.

robrap commented 2 years ago

I agree with @kdmccormick that I would be annoyed. When linting, we have control over whether we'd rather have more false negatives or false positives. I'd argue for more false positives, and see if the true positives end up being enough of a push in the right direction.

kdmccormick commented 2 years ago

Belated update: @e0d updated OEP-51 to disallow private links in commit messages unless explicitly marked with Private-ref: and messaged out the change.

This addresses the issue from a documentation angle, but I'm going to leave it open because we haven't checked whether there's been any improvement. If it's still an issue then we might need to write a linter to enforce compliance.

@nedbat , I recall you'd started experimenting with writing a linter, how did that go? I remember that our original assumption of "links that yield 4XX are private" was flawed because Jira (et al) returns 200 when permission is denied and just renders the error client-side.

nedbat commented 1 year ago

I started the linter, it's in a branch in repo-tools: https://github.com/openedx/repo-tools/tree/nedbat/private-link-linter. This is my repo where I'm trying it as an action: https://github.com/nedbat/python-action-private-links/tree/pr2

openedx / .github