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

SAM-238 - simplify GHA workflow #464

Open
wants to merge 85 commits into
base: develop
Choose a base branch
from
Open

SAM-238 - simplify GHA workflow #464

wants to merge 85 commits into from

Conversation

eapearson
Copy link
Contributor

@eapearson eapearson commented Apr 27, 2022

This set of changes replaces the existing GitHub actions workflow support consisting of 7 workflow definition files (yaml) and 7 supporting scripts (shell) with a 6 workflow definition files.

A new document, docs/gha/index.md describes the new workflow.

In brief, though, the new workflow takes advantage of GitHub's "reusable workflow" feature.

A reusable workflow is one which can be "used" by a primary workflow. Reusable workflows can only be triggered by a usage (on.workflow_call) and can define input parameters and secrets.

Two reusable workflows are defined, one for testing (test.yml) and one for building a service image and pushing it to GitHub Container Registry (GHCR) with a specific image tag (build-push.yml).

These reusable workflows are in turn used by four primary workflows which define the triggering conditions an set an image tag.

  • pull-request.yml handles normal PR workflow, running the tests on opening, reopening, and sychronizing a PR. It does not generate an image.

  • close-merge-pr.yml handles the specific case of closing and merging a PR into either the develop or master branches. It runs the tests and builds an image tagged with the target branch name (develop, master). The latter has the effect of ensuring that there is always a develop and master image containing the latest approved changes.

  • release.yml handles a release from master, running tests and building/pushing the image. It is tagged with the release tag, which should normally be a semver string in the format v#.#.# (although whatever tag used for the release will be used.)

  • manual.yml can only be run directly from the GHA user interface. It will run tests and build/push an image. It can handle many use cases, such as running directly against a feature (or fix) branch to build an image for evaluation, or on the develop or master branch if they need special non-PR merges.

The new workflow should also make it easier to reason about and make changes to the GHA process as the logic is quite easy to follow and all in one place.

There may be cases that are not handled the same, so this PR should serve as a place we can resolve these discrepancies.

It is not possible to actually review the workflows "in action" in the origin repo.

A new document docs/gha/evaluating.md provides instructions for evaluating the workflows in a fork.

@eapearson eapearson changed the title SAM-236 - simplify GHA workflow SAM-238 - simplify GHA workflow Apr 27, 2022
@eapearson eapearson marked this pull request as ready for review April 28, 2022 21:59
@bio-boris
Copy link
Collaborator

@jsfillman

@eapearson
Copy link
Contributor Author

I suspect that docker_build is a branch protection rule that may need modification for this PR or this PR may need modification to accommodate the rule.

@eapearson
Copy link
Contributor Author

@jsfillman more updates. IF build w/o push is supported, have separated out the reusable build workflow into two - one just to build and one to build and push. Although the core action this is built upon, "docker/build-push-action@v2", supports a push boolean option, it complicates a reusable workflow to support conditional usage, makes it easier to misuse the reusable workflow, and is generally simpler just to dedicate even the reusable workflow to one primary use case.

Also, replaced the shell scripted semver checker with a javascript one - more expandable, understandable by more devs, and is run by a nice general purpose, versioned, well-supported node script-runner action.

Single-use reusable workflows could be folded into the single workflow which uses then, reducing the # of files, at the cost of making those workflows a bit more complex. It is nice to look at the main workflows and see a simple set of steps and their dependencies with the implementation abstracted away.

Also, I've started expanding the docs.

Copy link
Contributor

@jsfillman jsfillman left a comment

Choose a reason for hiding this comment

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

@eapearson -- This looks really good and I'm excited to see it in action. LGTM!

@eapearson
Copy link
Contributor Author

Before we merge this, I'd like to run it through one full development cycle in a fork (will exercise all workflows and generate all images)

Also, @jsfillman or anyone else, can we resolve the "docker_build" required check above? I don't know what this is, but I did not it above in the comments:

I suspect that docker_build is a branch protection rule that may need modification for this PR or this PR may need modification to accommodate the rule.

@bio-boris
Copy link
Collaborator

I don't think we have documentation set up for running this in a fork.
That is correct, you can find those options in the branch protections area and change it there.

@@ -0,0 +1,20 @@
---
name: PR To master merged
Copy link
Collaborator

Choose a reason for hiding this comment

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

These rules should apply to main and master. The master branch should be renamed to main at some point

@@ -0,0 +1,18 @@
---
name: PR to master opened
Copy link
Collaborator

Choose a reason for hiding this comment

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

These rules should apply to main and master. The master branch should be renamed to main at some point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Yeah, I can make that change and test it on a fork which has a main.

@eapearson
Copy link
Contributor Author

I don't think we have documentation set up for running this in a fork.

I've done it because that is how I developed these workflows, and have added documentation for doing so in this PR.

That is correct, you can find those options in the branch protections area and change it there.

I can change it. I thought initially I didn't have admin access to the repo, but maybe I was looking at in while unauthenticated accidentally.

@bio-boris
Copy link
Collaborator

Added you!

@eapearson
Copy link
Contributor Author

I've added main branch support (in addition to existing master), and renamed scripts and docs to reflect "main" as the primary branch (noting that "master" is the legacy primary branch.)
Also converted diagrams from PlantUML to Mermaid. Mermaid does not support some of the plantuml features, so I don't find the diagrams as easy to ready, but they do work. I've left the plantuml diagrams in for comparison, but they can be removed now or later.
I've tested the appropriate branch protection rules in a fork, and Boris added me as admin to this repo, so I can adjust the rules later today to support this PR.

@bio-boris
Copy link
Collaborator

It looks like kbase/.github will be adding these reusable workflows.. Does that mean we should merge this in, and then immediately delete the workflows in favor of ones in .github?
Also it would be nice to get the testing instructions for a fork into the .github, can we make an issue in the .github repo @jsfillman or add it it jira if they are not already there.

@eapearson
Copy link
Contributor Author

It looks like kbase/.github will be adding these reusable workflows.. Does that mean we should merge this in, and then immediately delete the workflows in favor of ones in .github?

Is that a question for me? Sounds like a fair plain, although I'd perhaps rephrase that there should be a future change to replace local reusable workflows with the corresponding organization-wide workflows, and update documentation accordingly.

Also it would be nice to get the testing instructions for a fork into the .github, can we make an issue in the .github repo @jsfillman or add it it jira if they are not already there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants