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

E0d/namespace adr #36

Merged
merged 15 commits into from
Jan 8, 2024
Merged

E0d/namespace adr #36

merged 15 commits into from
Jan 8, 2024

Conversation

e0d
Copy link
Contributor

@e0d e0d commented Dec 7, 2022

An ADR to move forward this conversation that started in Discourse.

Copy link
Member

@kdmccormick kdmccormick left a 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.

docs/decisions/0001-namespacing-branches.rst Outdated Show resolved Hide resolved
docs/decisions/0001-namespacing-branches.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@feanil feanil left a 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?

docs/decisions/0001-namespacing-branches.rst Outdated Show resolved Hide resolved
docs/decisions/0001-namespacing-branches.rst Outdated Show resolved Hide resolved
@e0d
Copy link
Contributor Author

e0d commented Dec 8, 2022

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.

docs/decisions/0001-namespacing-branches.rst Outdated Show resolved Hide resolved
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
Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

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:

  1. 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).
  2. 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.
  3. 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?

Copy link
Member

Choose a reason for hiding this comment

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

  1. 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.

Copy link
Contributor

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:

  1. I think OEPs are the right way to communicate cross-repo decisions.
  2. 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.

docs/decisions/0001-namespacing-branches.rst Outdated Show resolved Hide resolved
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 10, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Oct 10, 2023

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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@mphilbrick211 mphilbrick211 added the inactive PR author has been unresponsive for several months label Oct 10, 2023
Comment on lines 83 to 84
4. A reasonable grace period should be established to schedule and
complete the required work
Copy link
Member

@kdmccormick kdmccormick Jan 3, 2024

Choose a reason for hiding this comment

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

@e0d @feanil @robrap

Suggested change
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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Member

@kdmccormick kdmccormick Jan 4, 2024

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.

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Member

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.

docs/decisions/0001-namespacing-branches.rst Outdated Show resolved Hide resolved
docs/decisions/0001-namespacing-branches.rst Outdated Show resolved Hide resolved
Comment on lines 83 to 84
4. A reasonable grace period should be established to schedule and
complete the required work
Copy link
Contributor

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.

kdmccormick and others added 2 commits January 3, 2024 10:53
@kdmccormick
Copy link
Member

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.

Copy link

@bmtcril bmtcril left a 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!

@kdmccormick kdmccormick merged commit ee52d7c into openedx:master Jan 8, 2024
1 check passed
@openedx-webhooks
Copy link

@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.

@nedbat
Copy link
Contributor

nedbat commented Jan 8, 2024

We ended up with two 0001 decision docs....!

@kdmccormick
Copy link
Member

Whoops, good catch. #110

@kdmccormick
Copy link
Member

FYI - I've started writing a repo check that will let us know which branches need to be deleted for Redwood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive PR author has been unresponsive for several months open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants