Docs-website: [Content] Allow Merges of content PRs from Relic Docs Team without approval

[jpvajda]

we only require 1 additional Approver of a PR for a merge on all PRs. If we had a list of content writers we could restrict the need for a PR approval process. maybe do this by a team policy in Github? There is a risk that something could be broken by a PR but it seems like a smaller risk. Could we route a PR based on type? Could this be done with Labels and Github Actions? - Take away: We probably want to do this, it's a matter of how.

AC

• [x] Setup Docs Team members to not require a PR approval for merges into Develop and Main branches
• [x] Perhaps we build the site in a GitHub action as a way to "smoke test" it.
• [ ] Austin will communicate and document how / when to engage the DevEx Hero if builds fail

[zstix]

At the moment, we require 1 approval before merging into develop and main (including administrators). To resolve this ticket, we would need to do the following:

1. Allow administrators to merge into develop without an approval
2. Update @newrelic/docs-team's role to be "Admin"

I still think it would be good to require an approval for merges into main (including administrators) so that the engineering team can review the site before we publish work.

If the above sounds good, I can make those adjustments.

[jpvajda]

@zstix Thanks for the info on this. I'm curious if there are any github bots or github actions that be used to be more explicit about when a merge can occur without an approval but I'm starting to think there isn't..

There is risk to not requiring approvals for both branches IMHO, and to try to do this just for content PRs vs. coding PRs seems difficult. also, content + code can be intermingled in PRs, so I'm leaning towards discussing with @austin-schaefer that he just builds a people process around this, and that we treat all PRs the same, where an approver is required, so we don't risk releasing something to the site that could cause an issue.

Furthermore, if we put more automated tests and checks in place, we could get to the point where this seems less risky, but at the moment I'm not 100% convinced we should allow this.

[austin-schaefer]

@zstix @jpvajda : Let's discuss more next week. Currently the writers publish (not edit, but publish) docs changes 20+ times per day. Requiring an approval every time someone fixes a typo or updates a link is going to create a lot of overhead, particularly for the hero process.

We also have a writer in Barcelona who won't have anyone in-timezone to approve PRs. It's important that she is able to publish changes in PST off-hours.

[jpvajda]

@austin-schaefer We've been discussing this need as a team so I'll include @jerelmiller in this thread, it's part of MMF11 -- I hear what you are saying but I think the question for me is: are content changes really that urgent where they can't follow a standard review process to ensure the code isn't released that could either trigger an on-call alert or break functionality? Code reviews of content PRs will also allow your team to enforce best practices, smaller commits, and catch potential bugs or problems in the content itself. I believe eventually with proper automated tests we could allow content to merge without reviews, but I'm a little apprehensive to do that until we have more tests in place that can ensure a merge won't break site functionality. Perhaps I'm being too overly cautious here, so I'll let @jerelmiller / @zstix state their opinions.

[jerelmiller]

@jpvajda I echo your concern about on-call. While MDX is great, it could be "easy" to introduce a syntax error, which would break the build of a page. While Amplify is good at not deploying broken code unless it builds, it does mean we'd need to keep a close eye on builds to make sure a content edit didn't introduce a broken build.

If we go this route, I'd want some way to ensure the build succeeds before the PR can be merged. Perhaps we build the site in a GitHub action as a way to "smoke test" it. While I'd love to rely on the PR builds from Amplify as this metric, unfortunately we've found that it can be sorta unreliable at communicating back to GitHub if a build succeeded/failed. I've noticed that sometimes it gets stuck in "pending" even though the build is finished. The GitHub action seems to be more reliable, even if it isn't ultimately the code that gets deployed to prod.

Let's just make sure that we have some kind of safeguard in place so that a broken build doesn't go unnoticed.

[jerelmiller]

As a followup question, I assume the idea for the "auto merge" would only apply to docs team members correct? We wouldn't want outside contribution to be able to merge without a review correct?

[austin-schaefer]

@jerelmiller : Yep, definitely only for docs team members.

[zstix]

We've set up a GitHub action to validate a pull request that will run tests and do a test build to make sure nothing introduced in the PR breaks the site. If that check fails, the PR will be blocked from merging in.

Now that that is in place, I have elevated the @newrelic/docs-team to the "Maintain" role which should allow members of that team to merge a PR in without a review (but it still requires the validation checks).

I'm going to move this ticket into "In Review" and assign it to @austin-schaefer. Let us know if you would like any additional adjustments to this setup. Y'all should be more free to merge stuff in!

[jpvajda]

@zstix this is awesome! thank you. It be cool to get this in place on the Developer Site as well, if it's not too much trouble.

[zstix]

@jpvajda I'll make a ticket in the dev site repo and maybe we can circle back to that in our catch-all (or during a sustainment period).

We're about to do a bunch of CI work in this MMF and it's possible that we might get even more cool stuff that can be used in that repo! 🎉

[austin-schaefer]

Awesome @zstix ! Pinging the writers to find someone to give this a whirl.

[bradleycamacho]

Hey guys, thanks for getting to work on this! I just made a test PR and ran into two roadblocks:

1. The Gatsby Build test took 42 minutes. Is this an expected length for the test? If so, is there any way we could try to shorten the build length? 40 minutes isn't a big deal for most content changes, but there are occasions where we need a faster content turnaround time.
2. This may be an error on my end, but I still can't seem to merge a PR without an approver. Maybe I haven't been granted the maintainer permissions yet?

• @zstix @austin-schaefer

[jerelmiller]

Hey @bradleycamacho! I can answer this question for you:

The Gatsby Build test took 42 minutes. Is this an expected length for the test?

Unfortunately, as of right now, yes this is the expected length. We'll need to do some digging on our end to see if we can optimize this in any way, but that likely won't happen before GA. It is definitely not ideal.

but there are occasions where we need a faster content turnaround time.

I'd be curious to understand some of these scenarios. Could you add some more detail about this?

[jpvajda]

Let's discuss @jerelmiller That lengthy build check for a PR is definitely negating the manual process we had in place without the check perhaps we can consider if there is a way to improve this prior to GA. Let's check this out..

incremental builds on amplify.. could be 🔥
https://bulletninja.com/2020-05-22-enable-gatsby-incremental-builds-aws-amplify/

[jpvajda]

Filed this: https://github.com/newrelic/docs-website/issues/553

[bradleycamacho]

Hey @jerelmiller,

Thanks for looking into this! I'll keep an eye out for any further work done on this.

but there are occasions where we need a faster content turnaround time.

There are times where content changes are needed ASAP. For example, we recently decided to rename a product and I updated the docs to reflect the new name. We later realized we could not use the name, and I had to edit the docs and change the name again as quickly as possible. I would predict our team would need this level of turnaround time at least a few times a month.

This may be an error on my end, but I still can't seem to merge a PR without an approver. Maybe I haven't been granted the maintainer permissions yet?

I'm still stuck on this section. My test PR has passed all the checks, but it still request a review before I am able to merge.

[bradleycamacho]

Hey @jerelmiller and @zstix, sorry to keep tagging you but I needed to raise a large issue I just ran into:

I noticed that the branch with my PR was "out of date" as of an hour ago. I clicked update so that it merge changes from develop, and noticed that it required all the checks to run again. I assume this would be true every time a PR is merged by a docs writer.

Since the checks take a total of ~45 minutes, this would mean that only ~12 content PRs would be able to go through in a workday. This isn't scalable since an individual writer regularly makes more than 12 edits in a single day.

Is there anyway to mitigate this?

[jerelmiller]

Hey @bradleycamacho! We've got a branch up right now that is experimenting with caching and incremental builds, which should hopefully speed it up by quite a bit. The caching alone seemed to cut down builds to ~8min. We don't have that merged yet, but hopefully soon when we've confirmed it works alright.

As for the requiring branches to be kept up-to-date, I think we'll probably disable that. Seems that could become a huge blocker here. Its good in theory, but I think its probably a better setting for other types of apps.

[jerelmiller]

Ok I just turned off the setting requiring that the branch be up-to-date

[bradleycamacho]

@jerelmiller Perfect, thanks for solving those issues!

[zstix]

We've removed any blockers for the docs team to merge in content and we've reduced the long build times to ~10 minutes. Improving the build times will be an ongoing effort, but for the purposes of this ticket, I'm going to consider this "done".

Thanks for the help everybody!