o3de / sig-release

8 stars 20 forks source link

23.05 Retros Action Items: Figure out improvements to reduce code submitted without following stabilization process #202

Closed vincent6767 closed 9 months ago

vincent6767 commented 1 year ago

Retros Action Item can be read here.

### Tasks
- [x] To add a task to the release project board for the "stabilization period" for the RM to communicate weekly to sig-all, reminding them of the stabilization process requirements (approvals, checklist, etc) with links
- [x] Reach out to Mike to see if it would even be possibly/realistic to further lock down the stabilization branch (only certain admins can merge a PR, for example).
- [x] Close this GHI once this completed: o3de/sig-build#96
vincent6767 commented 1 year ago

Here is the GHI that reminds the release manager to remind the Stabilization process in the SIG all channel: https://github.com/orgs/o3de/projects/51/views/1?pane=issue&itemId=32788207

vincent6767 commented 1 year ago

Hi @amzn-changml , The stabilization process in 23.05 still has some hiccups. For example, we had some code submitted without following the stabilization process because they didn't clearly understand the requirements (or where to find them). So SIG release wonders whether we can show the Exception checklist whenever someone creates a PR into the stabilization branch and only can be merged if these SIG approved (based on the checklist)

  1. UX SIG representative (chair/cochair)
  2. Testing SIG representative (chair/cochair)
  3. The code maintainers
  4. Release Manager or co-Release Manager.

Let me know your thoughts!

CC: @tonybalandiuk

amzn-changml commented 1 year ago

I'll investigate on branch specific PR templates. Github doesn't have direct support for this during the creation process, but it might be accomplished with a Github Action that triggers after the PR is made. I agree that some sort of exception checklist can at least signal to the user that they have to follow process.

In addition, Github recently added a feature to lock a branch to read-only. Previously, we would put a branch rule that would be very difficult to achieve (like needing 10 approvals), but this new feature could prevent inadvertent merges as we had during 23.05.

We do need someone to actually activate both of these functions, which we could achieve by assigning a team that can act as admins to the repo settings. Any suggestions on who should be on that team?

vincent6767 commented 1 year ago

Hi @amzn-changml Sorry for the late reply. Was having a hectic week.

In addition, Github recently added a feature to lock a branch to read-only.

I'm not sure how lock branch helps in this case cause that means no one can create a PR. Can you explain this further?

We do need someone to actually activate both of these functions, which we could achieve by assigning a team that can act as admins to the repo settings. Any suggestions on who should be on that team?

@amzn-changml I'm not sure what you mean by "activate". Are you looking for someone who will be the admin that could turn on / off the GitHub action and lock a branch feature for the stabilization branch?

amzn-changml commented 1 year ago

Apologies, I misunderstood. I was under the mistaken assumption that we were trying to block submissions that were outside the stabilization period. For the use case of having a checklist template for every PR in the stabilization branch, we would still need some way conditionally set the template based on the branch name. Failing that, we would need some sort of automation to add the checklist, but that might be after the PR has been created. For the latter case, someone may have to turn on this automation and set it to the appropriate branch name.

vincent6767 commented 1 year ago

we would still need some way conditionally set the template based on the branch name. Failing that, we would need some sort of automation to add the checklist, but that might be after the PR has been created. For the latter case, someone may have to turn on this automation and set it to the appropriate branch name.

I'm okay to have the checklist posted as a comment after the PR has been created. Can you help research and prototype these two possible solutions or is there another SIG that you think can help? @amzn-changml CC: @RoddieKieley @tonybalandiuk

amzn-changml commented 1 year ago

I'm okay to have the checklist posted as a comment after the PR has been created. Can you help research and prototype these two possible solutions or is there another SIG that you think can help?

Sure thing, I'll put it on SIG-Build's roadmap

tonybalandiuk commented 1 year ago

Wanted to weigh in here 1) I think it makes sense to have the checklist automatically posted to every PR against stabilization. 2) We should not have additional permissions to limit those merging PRs. Why? because there's already a limited set of users who can submit PR - we simply need to remind them of the guidelines and add "understanding Stabilization requirements" to their training/requirements

vincent6767 commented 1 year ago

Thanks. @amzn-changml ! Can you help paste the SIG Build Roadmap GHI here for tracking purposes? Also, is it possible to have this in place before September 11th? Just wondering whether we can have this by the next release which will happen in a month.

amzn-changml commented 1 year ago

Thanks. @amzn-changml ! Can you help paste the SIG Build Roadmap GHI here for tracking purposes? Also, is it possible to have this in place before September 11th? Just wondering whether we can have this by the next release which will happen in a month.

Sure, here's the GHI: https://github.com/o3de/sig-build/issues/96. I'll see about prioritizing this for this upcoming release. The investigative work will determine if we can have a solution in time.

vincent6767 commented 1 year ago

Thanks, @amzn-changml !

vincent6767 commented 1 year ago

I'll close this GHI once this completed: https://github.com/o3de/sig-build/issues/96

amzn-changml commented 1 year ago

Here's the automation PR: https://github.com/o3de/o3de/pull/16705. I've used the existing checklist found in the sig-release repo, but please feel free to request changes. An example of the comment can be found here: https://github.com/amzn-changml/o3de/pull/14

vincent6767 commented 1 year ago

That looks good to me. Very much appreciate it! Tagging @RoddieKieley @Ulrick28 in case they have any feedback.

A question @amzn-changml , what are the steps if a release manager or SIG Release want to update the checklist? Can they do it just by updating the doc in this link? https://github.com/o3de/sig-release/blob/main/releases/Process/O3DE%20Exception%20Checklist.md

amzn-changml commented 1 year ago

That looks good to me. Very much appreciate it! Tagging @RoddieKieley @Ulrick28 in case they have any feedback.

A question @amzn-changml , what are the steps if a release manager or SIG Release want to update the checklist? Can they do it just by updating the doc in this link? https://github.com/o3de/sig-release/blob/main/releases/Process/O3DE%20Exception%20Checklist.md

Correct, it will pull the current state of the file at the main branch

vincent6767 commented 1 year ago

Got it thanks! That means we need to create documentation in the repository that tells other people where to edit the checklist. I'll create one. Any suggestion on where that document should live? Is it on the SIG Release repo or O3DE? @RoddieKieley @Ulrick28 @Naomi-Wash @tonybalandiuk

Naomi-Wash commented 1 year ago

I vote SIG-Release repo @vincent6767 since it's a release-specific task.