But I can’t find information about the maintainer status of this, or if it’s still useful with how we manage code drift and release branches now? Do we still use it? What role/ticket is responsible with reviewing/merging the PRs?
@maxim do we want to update this to add our common ulmo branch of openedx-platform?
cc @Agrendalath as I see you’ve merged some of the sync PRs in the past.
@samuel, we never formalized this process. IIRC, we discussed passing this responsibility to the FFs, but I suppose we forgot to follow up. I used to merge these PRs when I had a moment, but then I had to work on older releases for a while, so I no longer had time and context to test the new ones. Also, resolving some of these merge conflicts became tricky over time, as we accumulated significant code drift.
I suppose it would be easier if we added Ulmo and removed everything else from there. Still, it would be nice to ensure somebody checks these PRs.
Still, it would be nice to ensure somebody checks these PRs.
Agreed! I wonder if this would be worth adding to some particular role. Perhaps to the community liaison, or at least to the community liaison to delegate? I’m easing into that role, and I see all these PRs now as I regularly check and keep tabs on everything.
I feel like to help deal with these kind of PRs, we need:
a quick way to determine which account we can log time to (it will be cumbersome to create a new ticket and discuss accounts for each new dependency update or code sync PR).
a quick way to find someone to delegate addressing it to.
If we have these, it would be easy for someone like the community liaison to eg. ping the FFs with “this Renovate PR just opened, please check and log time to BB-XXX”.
Handling these types of PRs could be considered a form of recurring maintenance, so we could create a recurring ticket for it in the DevOps Maintenance: Recurring Tasks epic and link it to the corresponding account.
Since the Community Liaison will often be the first person to see these PRs, adding the delegation process that you described to the scope of that role would make a lot of sense (and be preferable over introducing a new role just for this purpose)
–
As for the sync PRs, it sounds like we haven’t been merging them of late, at least not with any type of regularity/consistency. If that hasn’t caused any issues (?), we might not need them any longer and could consider disabling the automation that creates them.
If it turns out that we do still need them, it’ll definitely be worth talking some more about how to handle them and make sure they’re not missed going forward.
Maybe we’d want to handle them the same way as Dependabot/Renovate PRs, but since the context (Open edX release upgrades) is a bit different here I’m not sure.
We haven’t been merging them over the last few upgrade. I can’t evaluate whether we need this automation or not without looking deeper into it. For example, I would compare a branch from our fork for one of the previous releases against it’s upstream version (e.g. opencraft-release/sumac vs open-release/sumac) and inspect the commits that are present in the upstream but not in our fork.
I think where it might get tricky is if we want to split maintenance time to different accounts per-repository. For example, Renovate/Dependabot PRs for repos that are specific to a single client probably should come under that client’s maintenance account; and various internal projects are under different accounts (eg. SprintCraft, Listaflow, etc.).
From what I can tell, the intention is to keep our forked release branches up to date with new commits to the upstream release branches. This feels like a good thing to do, to reduce code drift and ensure we’re sticking close to upstream. Especially now since we’re moving toward using release branches for openedx-platform (eg. release/ulmo) rather than more static tags (eg. release/ulmo.1).
I guess the risk is that if we don’t have this kind of automation, then our release branches will fall behind and miss out on any backports/bugfixes from upstream.
Starting with just the main ticket under SE-6125 / Instances DevOps and using that for Renovate/Dependabot PRs against any repos that don’t contain/define any code/infrastructure that’s specific to a client or internal project.
Creating subtasks and linking them to client/project-specific accounts as necessary – i.e., if and when the first individual PR comes in that is specific to a client or internal project.
Putting a note in the description of the main ticket to mention that time should be logged on appropriate subtasks.
You’ll only need to create these subtasks once, and the full set will build over time.
(If you want to create a ticket for doing the initial setup and updating the Community Liaison role definition in the handbook, feel free to put me as reviewer.)
–
Re: sync PRs
@samuel@maxim I see where you’re coming from and I agree, keeping common/shared release branches up-to-date with backports and bug fixes from corresponding upstream branches seems like a good idea in principle. I.e., in the sense that it would make it easier for client owners to update their clients’ instances with those upstream improvements between upgrades.
How frequently client owners would actually do that, I’m not sure. For more mature instances, the number of redeployment opportunities tends to go down. Unless there’s a business case to be made (e.g. because a bug has been affecting users of a client instance directly, or a client needs some backported functionality to become available to their users as soon as possible), clients won’t necessarily want or need their instances to be redeployed just because a release branch got updated with additional changes.
Client owners would also need to track what’s happening in common/shared branches to know about bug fixes and backports being added to them in the first place.
So for the practice of tracking and merging sync PRs to be useful, it seems like there’s more that needs to happen than just the practice itself. And it would be worth making sure we’re not spending time/effort/budget on it just for the sake of keeping branches in sync
I see two options for moving forward here:
Using past data to get an idea of what we missed by not merging the sync PRs, as you suggested @maxim:
And/Or:
Picking the practice back up again (with some modification to keep client owners in the loop) and evaluating its usefulness at some point in the future; 6-9 months from now maybe.
For the second option, we could consider integrating it into the process for Renovate/Dependabot PRs discussed above: Time spent merging sync PRs could be logged on a subtask of the main ticket under SE-6125, which we’d link to the Instances Upgrades account.
This does make sense - stability over new features.
Was there a reason behind this decision, or simply that we never got around to merging them? Also, when you worked on checking for code drift on our openedx-platform branches, did you find the process impacted by not merging the sync PRs?
I’m thinking:
if checking for code drift isn’t impacted much
and it’s not affecting clients (because we manually backport any required security patches or requested features/fixes)
and we haven’t been merging them for a while anyway
then let’s delete this auto-syncing config altogether. If we discover we want it back sometime in future, then we can bring it back, with a related ticket, docs, and formal process to ensure they aren’t forgotten about.
Basically, the only time anyone would remember about these PRs was when I was checking the code drift dashboard. I think you can get some context here, but TLDR from my perspective: the automation has been created, but there wasn’t a process created for someone to be accountable or own the responsibility for checking these PRs, so it has been falling through the cracks. I wasn’t merging them as a part of the code drift check, because it was: 1) outside of the scope of the work I was doing; 2) it seemed like a lot of work, because many PRs have piled up and it seemed like a lot of effort to gather the context and test each PR (and I wasn’t confident merging them blindly).
It did clutter the code drift dashboard, but other than that - no.
I also discovered automated PRs from opencraft-requirements-bot titled chore: Upgrade Python requirements. Do you know where these come from? I haven’t been able to find the source. Also not sure how useful these have been.
While we’re looking at automated PRs, I’ve opened opencraft/dev/grove!414 that removes the custom renovate config from there. Would you mind taking a quick look? I’m 99% sure we don’t need that any more. cc @gabor
Re: PRs from opencraft-requirements-bot, I looked into it a little bit and what I found is that they might be coming from individual repos’ GitHub workflows:
(These files reference requirements_bot_github_* variables. Each of the repos targeted by one of the most recent chore: Upgrade Python requirements PRs has an upgrade-python-requirements.yml file.)
The bot itself seems to have been created in early 2023:
Automated dependency updates require a GitHub account, so we have created one. The credentials are specified in the org secrets (shared by all repos), as REQUIREMENTS_BOT_GITHUB_EMAIL and REQUIREMENTS_BOT_GITHUB_TOKEN. If you want to change any of these values, please ensure not to change the variable names, as this is the naming convention used by the upstream repos.
TBH, I don’t even remember which epic it was related to.
Back to the original theme of this thread - I’m still not sure who I should ping when I want to update our common branch. I opened this PR. It’s a clean Ulmo cherry-pick (with pylint fixes cherry-picked from master). Would anybody want to review it?
