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

Minimize PR overhead for non-code changes #16279

Merged
merged 9 commits into from
Dec 22, 2023

Conversation

JunTaoLuo
Copy link
Contributor

@JunTaoLuo JunTaoLuo commented Dec 4, 2023

Description

Resolves #13834. Here's a general overview of the design.

GHA workflows don't happen on documentation-only changes.

  • The GHA workflows are disabled using the paths-ignore mechanism.
  • .github/workflows/set-tugboat-tests-pending.yml was renamed to .github/workflows/set-tests-statuses.yml and its role is to set the correct status depending on whether the changes contain code or not. For Doc only PRs, it will set all the status for all required checks to success since the CI and Tugboat deployments are disabled. For PRs containing code changes, the current behaviour of setting va/test/* status to pending is maintained
  • I think it is valuable to keep the Contextual Advice workflow enabled since we may want to add advice based on which doc was updated (e.g. communicate to a Slack channel if a doc is updated, etc). As for the CodeQL and Repair PR Title workflows, I'm ambivalent but kept them for now.

Tugboat deploy doesn't happen on documentation-only changes

  • Current Tugboat integration uses webhooks to notify Tugboat of PR open, update and close. Since webhooks does not allow for flexibility of these triggers based on the types of file changed, webhooks were replaced with GHA workflows and Tugboat API. See discussion.
  • Since we need to retain the Preview ID between subsequent workflows (e.g. between creating and deleting the preview), actions/cache is used to persist the state.
  • On code change PR opened (both regular PRs and Draft PRs), .github/workflows/tugboat-pr-opened.yml will be triggered.
    • A call to the Tugboat API is made to create a preview for the PR.
    • The API response is analyzed and the preview ID is extracted to a .tugboat_response.txt.
    • The file is saved to the cache using the PR number as a cache key. The entry is created if new (on PR open) or refreshed if already existent (on PR reopen).
  • On code change PR updated or closed .github/workflows/tugboat-pr-{updated|closed}.yml` will be triggered.
    • The preview ID is restored from the cache using the PR number as a cache key.
    • A call to the Tugboat API is made to {rebuild|delete} the preview for the PR

Testing done

I've tested the changes involving both doc PRs and code PRs in the va.gov-cms-test repo:

I've confirmed that opening, updating and closing the PRs trigger the behaviour described above. Testing of reopening is completed in:

I've also tested the Tugboat Preview ID Cache refresh mechanism using department-of-veterans-affairs/va.gov-cms-test#1462 and confirmed that it ensures the Preview IDs for open PRs are refreshed on a set schedule.

QA steps

Testing this change requires turning off the webhook so it can't be done in this repo. The changes in this PR are mirrored at https://github.com/department-of-veterans-affairs/va.gov-cms-test/tree/13834-mirror. To test these changes, make a PR to that branch similar to the ones linked in the Testing done section.

As any user who can open PRs on this repo

  1. For a PR containing code changes (with or without documentation changes)
    • The Github Actions based CI workflows are triggered and completes as before
    • Tugboat deployments are triggered correctly
      • Previews are created on PR open
      • Previews are rebuilt on PR updates, ensure the SHA of the commit used to build the preview is the updated commit.
      • Previews are deleted on PR close
      • Previews are recreated on PR reopen
  2. For a PR containing only documentation changes (i.e. without code changes)
    • The Github Actions based CI workflows short circuits and completes quickly
      • Most checks will still run but should complete within 15 seconds compared to >= 1 minute if they didn't short circuit
    • The Tugboat deployments are not triggered
    • The required checks are marked as passed so merging is unblocked

Definition of Done

  • Documentation has been updated, if applicable.
  • Tests have been added if necessary.
  • Make an announcement in Slack for this change in Tugboat Preview behaviour.
  • Automated tests have passed.
  • Code Quality Tests have passed.
  • Acceptance Criteria in related issue are met.
  • Manual Code Review Approved.
  • If there are field changes, front end output has been thoroughly checked.
  • Once this change has been merged to main, the Tugboat PR webhooks on this repo can be disabled.
  • Refresh the TUGBOAT_API_TOKEN with an official token. This will be done in a followup ticket.

Select Team for PR review

  • CMS Team
  • Public websites
  • Facilities
  • User support
  • Accelerated Publishing

@JunTaoLuo JunTaoLuo added the DO NOT MERGE Do not merge this PR label Dec 4, 2023
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 4, 2023 02:33 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 4, 2023 02:40 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 4, 2023 02:53 Destroyed
@github-actions github-actions bot added the CMS Team CMS Product team that manages both editor exp and devops label Dec 4, 2023
@JunTaoLuo JunTaoLuo force-pushed the 13834-minimize-doc-pr-overhead branch from 49669b5 to 9365c4d Compare December 7, 2023 03:55
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 7, 2023 03:55 Destroyed
@JunTaoLuo JunTaoLuo force-pushed the 13834-minimize-doc-pr-overhead branch from 9365c4d to f4658e6 Compare December 7, 2023 03:59
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 7, 2023 03:59 Destroyed
@JunTaoLuo JunTaoLuo force-pushed the 13834-minimize-doc-pr-overhead branch from f4658e6 to b3e1ba1 Compare December 9, 2023 02:19
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 9, 2023 02:19 Destroyed
@JunTaoLuo JunTaoLuo force-pushed the 13834-minimize-doc-pr-overhead branch from c384410 to a12ee32 Compare December 9, 2023 02:40
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 9, 2023 02:40 Destroyed
@JunTaoLuo JunTaoLuo closed this Dec 9, 2023
@JunTaoLuo JunTaoLuo reopened this Dec 9, 2023
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 9, 2023 02:41 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 9, 2023 02:41 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 9, 2023 02:47 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 9, 2023 02:47 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 9, 2023 02:47 Destroyed
@JunTaoLuo JunTaoLuo removed the DO NOT MERGE Do not merge this PR label Dec 9, 2023
@JunTaoLuo JunTaoLuo changed the title [DO NOT MERGE] Minimize PR overhead for non-code changes Minimize PR overhead for non-code changes Dec 9, 2023

for (const file of files) {
console.log(`Checking PR file: ${file}`)
if (!file.endsWith('.md')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently we use the rule that any non-.md file is a code change, which is probably the safest method for now.

@JunTaoLuo JunTaoLuo marked this pull request as ready for review December 9, 2023 03:46
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 21, 2023 20:34 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 21, 2023 20:35 Destroyed
@JunTaoLuo JunTaoLuo force-pushed the 13834-minimize-doc-pr-overhead branch from b939612 to e91ef24 Compare December 21, 2023 23:24
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 21, 2023 23:24 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat December 21, 2023 23:24 Destroyed
Copy link

github-actions bot commented Dec 21, 2023

GitHub Workflows (.github/workflows/*.yml)

Have you...

  • pinned all affected GitHub Actions at a specific commit by SHA?
  • reviewed the source code of the action at the commit you are pinning?
  • confirmed that no GitHub security measures are being bypassed?
  • checked for any injection of user content into protected contexts?
  • reviewed Security hardening for GitHub Actions?
  • reviewed GitHub Workflows?

@JunTaoLuo JunTaoLuo requested a review from ndouglas December 21, 2023 23:47
Copy link
Contributor

@ndouglas ndouglas left a comment

Choose a reason for hiding this comment

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

Fantastic work!

@JunTaoLuo
Copy link
Contributor Author

Thanks everyone!

@JunTaoLuo JunTaoLuo merged commit 176aa38 into main Dec 22, 2023
19 checks passed
@JunTaoLuo JunTaoLuo deleted the 13834-minimize-doc-pr-overhead branch December 22, 2023 00:02
@JunTaoLuo JunTaoLuo mentioned this pull request Jan 8, 2024
5 tasks
JunTaoLuo added a commit that referenced this pull request Jan 24, 2024
JunTaoLuo added a commit that referenced this pull request Jan 24, 2024
@JunTaoLuo JunTaoLuo mentioned this pull request Feb 1, 2024
32 tasks
JunTaoLuo added a commit that referenced this pull request Feb 1, 2024
JunTaoLuo added a commit that referenced this pull request Feb 2, 2024
JunTaoLuo added a commit that referenced this pull request Feb 2, 2024
* Revert "Add documentation for Tugboat GHA integration (#16739)"

This reverts commit e5bce05.

* Revert "Refresh Tugboat Preview ID cache from PR cache scope (#16562)"

This reverts commit 48de27e.

* Revert "Minimize PR overhead for non-code changes (#16279)"

This reverts commit 176aa38.

* Update documentation (#17123)

* Test PR with documentation change

* Update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMS Team CMS Product team that manages both editor exp and devops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minimize PR overhead for non-code changes.
4 participants