-
Notifications
You must be signed in to change notification settings - Fork 27
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
E0d/namespace adr #36
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is very reasonable.
LGTM, pending a style nit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as well. I'd love a more concrete grace period but I defer to you on how feasible that is. Maybe we can put in an upper limit of a year?
I agree. I'd like some input from JB on that. If I don't hear on the Discourse thread, I'll reach out in other ways. |
and tags to clarify their origin and purpose | ||
3. The global namespace will be reserved for global project purposes | ||
|
||
Going forward branches and tags that originate from any specific |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@e0d: If this a global decision, it should really be an OEP and follow the OEP proposal process. In this case, this would probably just serve as a useful inform. At one-time, we even had a template (or note) about an OEP that could just refer to an ADR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that this ADR is sufficient for gathering input, and sharing it out via Slack will be a sufficient inform. Once this lands, I'll open a ticket to add these rules to the Core Contributor course if they're not already there. Can you folks at 2U add these rules to your own dev onboarding docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm realizing that I'm not entirely clear on the rules, and thus what belongs in our onboarding docs. I'm confused because I don't know how complete the examples are.
Here are some questions/thoughts:
- It feels like the consequence including "...guidance that personal branches..." should be part of the decision.
- It would be helpful to get clarification about what is and what is not included in "...branches and tags that originate from any specific organization...".
- According to the consequences, it sounds like personal branches are not included, even as employees of some organization.
- Also, is
release
the only in-use and illegal branch and tag name? Is this just an example, or is this the problem? Would it be valid to reword to "... release branches and tags that originate..."? If not, are there other examples or clarifying description to add regarding what this is referring to?
Separate but related, it would be interesting to see a spreadsheet of repos, tags, and branches that are at issue here. I'm not clear what the actual impact is to 2U, and whether this impacts many repos, branches and tags, or if it just affected edx-platform, and if this has been cleaned up already? This is related to the doc request, because I'm not clear about what messaging, if any, would be appropriate for most engineers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm realizing that I'm not entirely clear on the rules, and thus what belongs in our onboarding docs. I'm confused because I don't know how complete the examples are.
I think the ADR is clear. Feel free to open a PR against this one if you think it could be organized or worded better.
According to the consequences, it sounds like personal branches are not included, even as employees of some organization.
Correct.
Also, is release the only in-use and illegal branch and tag name? Is this just an example, or is this the problem? Would it be valid to reword to "... release branches and tags that originate..."? If not, are there other examples or clarifying description to add regarding what this is referring to?
Philosophically, we would like 2U and CCs to act in good faith and namespace their branches appropriately, always. In practice, we don't plan to be jerks about it; would give a heads up and sufficient time to adapt before deleting anything. There are dozens of branches and tags which conflict with this ADR, but release
and release/*
are the only ones which pose a concrete problem right now, so they are the only ones we are hoping to delete as early as March.
Separate but related, it would be interesting to see a spreadsheet of repos, tags, and branches that are at issue here. I'm not clear what the actual impact is to 2U, and whether this impacts many repos, branches and tags, or if it just affected edx-platform, and if this has been cleaned up already?
See #36 (comment) for branches. I'll re-ping later once I'm able to update those JSON docs to also include tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the documentation ask:
Generally, we just need to make sure that people with upstream write access understand this guidance, since I don't expect new devs to be looking through .github ADRs. Currently, there are only two groups with upstream write access: CCs, and 2U. I can take care of making sure that CCs see this guidance, probably via the CC onboarding course. What we need from 2U is to make sure their employees understand it as well, since they won't all be taking CC onboarding. However you think it'd be best to communicate that, and whether it's public or private, is fine by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kdmccormick: Thank you for adding the clarity I needed. I was having trouble determining whether the ADR was focused on the concrete problem or the philosophical problem, especially as it related to any deadline.
Regarding a variety of points from the above thread:
- I still have no idea why this isn't an OEP. There are benefits to that process, and I still don't get why this would be an exception, or treated any differently from conventional commits (as an example).
- I think it would be more clear and helpful to others to call out the reserved keywords like "release" that will be targeted for deletion, from all other branches which are philosophically a problem, but aren't causing any concrete issues at this time. Or maybe the ADR could note different types of treatment for branches created before or after a certain date? I'm just looking for simple ways to codify in the text the following sentiment: "In practice, we don't plan to be jerks about it.", especially given the number of non-namespaced branches/tags.
- New topic (related to documentation): [food for thought] I think one of the most helpful things for any OEP like this is automation, like that used for conventional commits. I don't know if there are ways to prevent new branches/tags that don't match the convention? If not, maybe a job could detect new issues and simply notify users? Maybe just a comment on PRs with non-namespaced branch names would be enough to help spread the word?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- An OEP is a greater time investment than an ADR. It takes longer to write and review. It invites all sorts of comments around grammar and formatting and "What about $RELATED_PROBLEM?". I think that extra time investment into consensus-building is worthwhile it when the decision at hand is especially important, impactful, controversial, and/or complex. In this case, though, we are talking about something that is, in the grand scheme of things, unimportant--the connection between branch names and learner outcomes is very indirect. It's also not controversial--this has been an unwritten best-practice as long as I can remember working on this project. It's not complex either--I don't think that protracted discussion with a wide array of community members will yield a substantially better decision. So, it's not only that I don't want to make this an OEP; I believe it would actually be an irresponsible use of my employer's time, which the community is counting on to be spent delivering deeply-needed improvements to the platform itself.
2+3) Your comments have made me realize that this ADR is actually being too specific. I'm going to remove the mention of the March 11 deadline. This ADR is simply deciding that branches should be namespace by GH username or organization. Once this merges, we will make a judgement call on which branches will be removed when, and communicate that out with ample time for 2U to adjust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. You don't have to change gears, but some thoughts regarding OEP:
- I think OEPs are the right way to communicate cross-repo decisions.
- In my opinion, there is nothing fixed about the OEP process that forces it to take a greater time investment. You could simply note in the PR or announcement that you are mostly documenting a past decision, and that you are not leaving it open for input, etc. Anyway can always fix typos or adjust in the future.
3. Related, we wanted to make OEPs quick to write, so there is a template that allows you to refer to an ADR. So, even once this lands, someone could loop back with an OEP that points to this decision, and doesn't open the door for additional overhead, simply to allow it do be documented in the expected location.
Note that I've short-circuited DEPR process details when I knew I was going to DEPR something for my own reasons, but I still wanted to use it for the inform.
In any case, all food for thought. Thanks and good luck.
Thanks for the pull request, @e0d! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Co-authored-by: Robert Raposa <[email protected]>
4. A reasonable grace period should be established to schedule and | ||
complete the required work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4. A reasonable grace period should be established to schedule and | |
complete the required work | |
4. The community will be informed that all personal and organizational branches | |
and tags must be namespaced before 2024-03-11, which is approximately one | |
month before the Redwood release branches are to be cut. | |
Starting on 2024-03-11, Open edX project administrators may begin deleting | |
un-namespaced branches and tags, particularly those which conflict with community | |
processes such as the named release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update.
FYI: @jmbowman: In case you want to discuss this deadline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to leave this suggestion open for comments, but I plan to apply it before merging if there are no objections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kdmccormick: Are you planning on scripting a large clean-up, or just proposing clean-up of specific known problematic branches or tags? I'm unclear how much work will be involved, if any, and that would dictate how problematic any deadline might be. Do you have information around this? Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we will clean up in March are branches and tags matching release
or release/*
.
We may want to clean up other branches and tags in the future, but we wouldn't do so without a separate heads-up and grace period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This last comment of yours is much more targeted and less scary than the text of the ADR, especially since you've produced a list of potential target branches. Can you update the ADR to match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in #36 (comment), I've gone the other direction and removed details. Announcements to follow with clearly state which branches up for deletion and what the grace period is.
4. A reasonable grace period should be established to schedule and | ||
complete the required work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update.
FYI: @jmbowman: In case you want to discuss this deadline.
Co-authored-by: Robert Raposa <[email protected]>
Co-authored-by: Robert Raposa <[email protected]>
Here are three JSON files:
As I write this comment I realize I didn't include tags. I'll circle back later update those JSON files to include both branches and tags. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this over the line, lgtm!
@e0d 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
We ended up with two 0001 decision docs....! |
Whoops, good catch. #110 |
FYI - I've started writing a repo check that will let us know which branches need to be deleted for Redwood. |
An ADR to move forward this conversation that started in Discourse.