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

Upgrade upload-artifacts GHA step to v4 #29

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danschmidt5189
Copy link
Contributor

@danschmidt5189 danschmidt5189 commented Dec 3, 2024

@anarchivist @yzhoubk This one's ready to go, but given the significance of the change I've opted not to merge it til you've had a chance to review it and I've returned from vacation (I'll be back Jan. 6). Please take a look at this commit and the .github repo I mention below (which you should both have access to). I'll be out next week but am happy to glance at some comments during my downtime if you have them, so don't worry about bugging me!

Workflow Changes

This converts GeoData's CI process to the docker-ci workflow template from BerkeleyLibrary/.github. See GitHub's workflow template for documentation of that underlying GHA feature. I'll focus on what's novel to my implementation.

A key point: Workflow Templates make it easy to setup CI for new repos just by clicking a few buttons in the GitHub UI. So, this overall change doesn't just improve GeoData's CI/CD, it will help to improve and standardize CI/CD for all of our GitHub repos.

This breaks CI into two steps:

  1. Build, which just builds, tags (SHA, branch, tag), labels, and pushes the Docker image to ghcr.io/{repository}.
  2. Test, which tests the built image (identified by SHA; this could be improved!) using docker compose.

Expectations / Limitations

For building, note that this V1 implementation just builds and pushes everything automatically—before testing. This is fine so long as we follow our usual pull request workflow. For staging deployments, the image would have been tested as part of the PR test; for production deployments, it would have been tested via a complete build/test run of the underlying commit. But this is something to discuss / explore, and I think it would be best to enforce this in V2 of the workflow.

For testing, the V1 requires that the consuming repository implement a few files:

  1. docker-compose.yml/docker-compose.ci.yml, which are overlaid / merged to form the CI stack in which the application is tested.
  2. bin/setup, an executable that sets up the application prior to testing.
  3. bin/test, an executable that runs tests and outputs the report to STDOUT.

Both (2) and (3) should be on the PATH of the built application container.

GitHub Actions support arguments / parameterization, so in principle the setup, test, and Compose File(s) could be customizable. I just opted not to do that for the V1.

Solr

To bootstrap solr with the correct core configuration(s), I created a solr/Dockerfile and modified docker-compose.yml to actually build solr rather than just pull it from DockerHub. This pretty elegantly solves the bootstrapping problem, since now there's no need to hackily inject core configs at runtime—they're present in the image, which besides that is identical to the image we'd been using before.

Migrates to a new, shared GitHub Actions workflow template provided by the BerkeleyLibrary/.github repository. This does essentially what we have always done, but pulls the workflow configuration from the central repository. The CI process is as follows:

- Build: The Docker image is built, tagged (branch, SHA, and tag), and pushed to ghcr.io/berkeleylibrary/geodata.
- Test: The built image is tested using `docker compose`.

The test stage depends on the consuming repository having implemented `bin/test`, `bin/setup`, and `docker-compose(.ci)?.yml` files. For more information on that, see the accompanying files in this commit or the forthcoming documentation in the shared workflows repo.
@danschmidt5189 danschmidt5189 force-pushed the DEV-769-upgrade_artifact_uploads_action branch from 43ac585 to 8f857a5 Compare December 27, 2024 23:58
Copy link
Member

@anarchivist anarchivist left a comment

Choose a reason for hiding this comment

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

this looks great! only a few questions and comments

Copy link
Member

@anarchivist anarchivist left a comment

Choose a reason for hiding this comment

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

sorry, forgot to actually stage up the questions/comments, the remaining of which are here.

  • with these changes, am i correct to understand that we no longer need solr_wrapper as a dependency? if that's the case, i think we can remove it from the Gemfile (and subsequently Gemfile.lock).
  • i've noticed the build jobs on BerkeleyLibrary/.github are failing. my guess is that we're not expecting that repo to build an image of itself. if that's the case, can we add a condition along the lines of if: github.repository != 'BerkeleyLibrary/.github' to the build job within docker-build.yml?

end

FileUtils.chdir APP_ROOT do
system! "rspec -f html"
Copy link
Member

Choose a reason for hiding this comment

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

do we want to run the same command to run the tests that were in the previous build workflow definition? these were as follows

      - name: Run tests
        env:
          RAILS_ENV: test
        run: bundle exec rake check -t

      - name: Run style checks
        run: bundle exec rubocop

      - name: Validate database migrations
        env:
          RAILS_ENV: test
          DISABLE_DATABASE_ENVIRONMENT_CHECK: 1
        run: rails --trace db:drop db:create db:migrate

@yzhoubk
Copy link
Contributor

yzhoubk commented Jan 4, 2025

The workflow makes sense. I tested this branch locally, the solr instance works well.

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