SAM-238 - simplify GHA workflow

[eapearson]

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.

[bio-boris]

@jsfillman

[eapearson]

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]

Pinging @jsfillman for a review :)

[jsfillman]

Reviewing now to make sure the base devops requirements are met:

1. An image named {REPONAME}-develop:pr# is created when a PR to develop is first created.
2. An image named {REPONAME}-develop:latest is created/tagged when a PR to develop is merged.
3. An image named {REPONAME}:pr# is created when a PR to main (master) is first created.
4. An image named {REPONAME}:latest-rc is created/tagged when a PR to main (master) is merged.
5. A final release image is published using the Draft a new release functinality, which results in an imaged tagged as {REPONAME}:x.x.x and {REPONAME}:latest.
6. An optional on-demand workflow can be used to trigger image builds from any branch at any time if desired.

[jsfillman]

I'm loving how clean and easy to read this is. Great work @eapearson!

As far as meeting specific devops requirements:

• We do need a separate {REPONAME}-develop image for any builds against the develop branch.
  • I know, it's not a super popular feature, but we need it for CI/CD
  • This also keeps the primary image "clean" in that you know all images were built off of main.
    • The only tags in the main {REPONAME}: image should be :pr#, :latest-rc, prod release (:x.x.x), and :latest (most recent release image)
• Images should be generated both before and after merge
  • This is to ensure code that breaks builds never makes its way from a feature branch to develop (or worse develop > main!)
  • This also enables testing images for bugs prior to merge
• The release workflow needs to both check for appropriate semantic versioning (x.x.x) and require the release occur from main (master).
  • You could probably take the checks from the start of https://github.com/kbase/.github/blob/main/workflow-templates/prod_release.yaml and just replace the bottom portion with your code

Lastly, since we're talking reusable workflows, I'm of the opinion we should give reusable_test.yml a more verbose name, maybe reusable_test-python.yml or similar (not a hard requirement, just a nice to have).

Note that all of the template workflows (available through Actions > New workflow button) already meet the discussed requirements, but are not as elegantly executed as your code, although we're actively improving that now.

For now, if we can get the above requirements met, I'm happy to give this a huge 👍 and likely integrate some of this code into our proposed template cleanup.

[eapearson]

Reviewing now to make sure the base devops requirements are met:

1. An image named {REPONAME}-develop:pr# is created when a PR to develop is first created.
2. An image named {REPONAME}-develop:latest is created/tagged when a PR to develop is merged.
3. An image named {REPONAME}:pr# is created when a PR to main (master) is first created.
4. An image named {REPONAME}:latest-rc is created/tagged when a PR to main (master) is merged.
5. A final release image is published using the Draft a new release functinality, which results in an imaged tagged as {REPONAME}:x.x.x and {REPONAME}:latest.
6. An optional on-demand workflow can be used to trigger image builds from any branch at any time if desired.

Gotcha, I'll update to ensure images are created thus.

[eapearson]

@jsfillman Should the image generated for the develop branch at PR creation ("opened") event be updated as the PR is updated? To do this in other workflows I've triggered on the actions "opened", "reopened", "synchronized". If one is using an image for a PR, shouldn't it reflect the current state of the PR?

[eapearson]

@jsfillman Also, I'm not sure I understand the utility of creating images for every PR. I do see the usefulness of generating images from PRs, e.g. I do it from time to time for kbase-ui to test an image for a short time before merging to main (which is usually the image deployed in CI - but kbase-ui does not use a develop branch, so it is simpler.)

However, I don't think that is a commonly (ever?) done for sample_service, and seems less useful for highly collaborative projects like sample_service since there are so many open pull requests and there is no good candidate for deployment.

An alternative is to just test PRs.

If a specific set of changes needs to be evaluated via an image, won't the manual workflow run be sufficient if the source branch is up-to-date with develop? That could be done on a case by case basis if some important PR should be deployed for evaluation.

[jsfillman]

@jsfillman Also, I'm not sure I understand the utility of creating images for every PR. I do see the usefulness of generating images from PRs, e.g. I do it from time to time for kbase-ui to test an image for a short time before merging to main (which is usually the image deployed in CI - but kbase-ui does not use a develop branch, so it is simpler.)

However, I don't think that is a commonly (ever?) done for sample_service, and seems less useful for highly collaborative projects like sample_service since there are so many open pull requests and there is no good candidate for deployment.

An alternative is to just test PRs.

If a specific set of changes needs to be evaluated via an image, won't the manual workflow run be sufficient if the source branch is up-to-date with develop? That could be done on a case by case basis if some important PR should be deployed for evaluation.

@eapearson you make good points. The idea is to make sure that it does build as a prerequisite test for merging. However, you could make the point that if it doesn't build, it can't be deployed! 😆 Let me see how strongly the rest of the team feels on omitting the build on open... a decent compromise might be to only to only do pre-merge builds on PRs from develop > main, which would only be done when ready for a release.

[eapearson]

@jsfillman Also, I'm not sure I understand the utility of creating images for every PR. I do see the usefulness of generating images from PRs, e.g. I do it from time to time for kbase-ui to test an image for a short time before merging to main (which is usually the image deployed in CI - but kbase-ui does not use a develop branch, so it is simpler.) However, I don't think that is a commonly (ever?) done for sample_service, and seems less useful for highly collaborative projects like sample_service since there are so many open pull requests and there is no good candidate for deployment. An alternative is to just test PRs. If a specific set of changes needs to be evaluated via an image, won't the manual workflow run be sufficient if the source branch is up-to-date with develop? That could be done on a case by case basis if some important PR should be deployed for evaluation.

@eapearson you make good points. The idea is to make sure that it does build as a prerequisite test for merging. However, you could make the point that if it doesn't build, it can't be deployed! 😆 Let me see how strongly the rest of the team feels on omitting the build on open... a decent compromise might be to only to only do pre-merge builds on PRs from develop > main, which would only be done when ready for a release.

The only concern I have with the image building for PRs is the proliferation of images which someone will need to prune at some point, unless there is a workflow which deletes PR images. It really is trivial in terms of the workflow whether a build occurs or not.

I'm more concerned with tightening up the PR testing. Triggering on "reopened" ensures that this edge case is handled. Triggering on "synchronize" ensures that the PR is always tested as it is iterated upon.

[eapearson]

It is a fresh morning, and I now have better read the requirement for a "proof of build" before merging. I agree! In such cases where proof of build is required but an image will not be used (and will complicate life of maintainers) we can perform the image build, but not push the image to GHCR. I'll incorporate that into the reusable workflow as a proof of concept, at least.

[eapearson]

I've updated the branch and thus PR with the latest changes. Notably

• should be mostly compliant with the requirements, other than making the develop pr workflow also triggered by reopen and synchronize and develop pr does not push an image (does build though)
• a new reusable workflow for semver validation
• the build-push reusable workflow takes "push" and makes "tag" an input so that it can be skipped
• build-push uses a script step via actions/github-script@v6 to munge the tag so that multiple tags can be passed (to support release with two tags); confusing since image tags are not exactly the same thing as the docker/build-push-action@v2 tags.

I've run all workflows during this process, but have not run them all on the precise last set of changes. I'll do that later and report back here.

[eapearson]

Also, I should read through the requirements another time and double check everything. Again, I can do that later, most probably over the weekend.

[eapearson]

@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.

[eapearson]

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]

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.

[eapearson]

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]

Added you!

[eapearson]

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]

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]

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.