Add merge and review policy #220
Conversation
choldgraf
force-pushed
the
merge-policy
branch
2 times, most recently
from
4c5d704
to
5f4c8ca
Compare
choldgraf
force-pushed
the
merge-policy
branch
from
5f4c8ca
to
cbd0bb8
Compare
practices/development.md
Outdated
|
|
||
| ### Policy for terraform changes | ||
|
|
||
| Changes that provision active hub infrastructure (e.g., almost anything involving `terraform`) have a different workflow that requires different policies. |
There was a problem hiding this comment.
Since we automatically deploy all hubs with continuous deployment in the pilot-hubs repo on every merge, this applies to all config changes - not just terraform.
There was a problem hiding this comment.
ok cool - will re-work to include "all active JupyterHub changes"
practices/development.md
Outdated
| Each time you do, make sure the PR is updated with the state of the code when you deployed, and leave a comment making it clear to others that a deploy has been made. | ||
| - **Use a draft PR before you have deployed something**. | ||
| This will signal to other members of the team that you have not changed any infrastructure yet, and are merely asking for feedback. | ||
| - **Be more hesitant to deploy/merge without approval**. |
There was a problem hiding this comment.
Most things we do in pilot-hubs are difficult to test without deploying, so I think we should relax this somewhat. I split this into two parts (new infra / existing) in 2i2c-org/infrastructure#516.
For most PRs, running it + seeing the response from that is going to be far far more important and useful than reviewing what's actually written. I suspect cases like 2i2c-org/infrastructure#613 will be common, where the code itself is incidental to the effects it has on deployment. We can't really wait for approval of the code before running it each time.
I don't really have a suggested alternative tho.
There was a problem hiding this comment.
ah jeez I totally forgot that this PR exists...hmmm do you think it's better for us to move this section on "active infrastructure" to the pilot-hubs/ repository and then cross-ref it from here? I could push to your PR if you like.
My reasoning for having it in team-compass/ is that currently we do have a few non-pilot-hubs/ repositories for infrastructure, but I could see that repository being a more natural place for people to look. What do you think?
There was a problem hiding this comment.
What I have written here is, I think, relevant for this discussion...
There was a problem hiding this comment.
I did a bunch of work over in mybinder.org-deploy on deploying to a staging environment in a PR - most recently this PR which gets around the fact that if you've opened a PR from a fork, secrets are not available by default in GitHub Actions jupyterhub/mybinder.org-deploy#2002
I'd be very happy to do something similar for the pilot-hubs repo
There was a problem hiding this comment.
Super interesting work, @sgibson91! Regardless of the implementation details (I would love to see it in the direction you are showing in the work you referenced above), I think the concept of having some staging environment and how that plays out with the merging and review policies is the thing we may want to further discuss, more probably, in another issue and once we have some prototype guidelines already working.
There was a problem hiding this comment.
Yes, to clarify, I was thinking on the hubs level with my above links, not on the cluster level which is now being discussed in #228 :)
|
OK all thanks so much for the feedback! After realizing that @yuvipanda had another PR where he was iterating on this, I decided to update that PR with some guidelines around infrastructure and review, and I've changed this PR to refer to @sgibson91 and @damianavila I think that it would be great if we could find some way to properly operate a staging cluster for our changes before they went live. Is that worth opening a separate issue to track? It seems like it might be a bit complex, given that we do not have a single hub, but a federation of many slightly different hubs. |
I think a separate ticket for that discussion is OK. I would love to see some guidelines in place ASAP and then iterate on those taking into consideration the staging > production concepts. |
Co-authored-by: Damian Avila <[email protected]>
|
@damianavila I opened up an issue to discuss staging hubs here: #228 |
|
ok merging this one in so that we can focus our attention on the meatier proposal in 2i2c-org/infrastructure#516 |
This is a proposed merge / review policy for our team's "non-infrastructure" changes! It also describes some of our decision-making process more generally!
For changes to active infrastructure, this PR refers to pilot hubs (and related PR: yuvipanda/pilot-hubs#1)