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

Simplify the current CI/CD workflow #10

Merged
merged 14 commits into from
Dec 5, 2023
Merged

Conversation

drmrd
Copy link
Contributor

@drmrd drmrd commented Nov 29, 2023

Highlighted Changes

A common Docker image version is now declared in a config file

A .env file is now in the root directory of the repository. For each commit on main, this is intended to contain the semver of the first release containing the changes in that commit.

  • Example 1: In the commit tagged as release v0.0.3, the .env will contain 0.0.3.
  • Example 2: For all subsequent commits up to and including the commit tagged as v0.0.4, .env should contain 0.0.4.

The pattern in these examples can continue after we move past 0.0.* releases, but the hope is that we will consolidate these Docker images with others from everest-utils and other EVerest repositories in a location that's separate from the Docker Compose files defining these demos. This would obviate the need for storing version information in the .env file and further streamline the overall release process.

The version specified in .env should also match the version of each image in our Docker Compose files going forward.

Changes to our CI/CD workflow

The GitHub Actions cicd.yaml workflow has been updated to rely on the .env file for version information. The intent here is to simplify our existing build processes, which were put in place to address a chicken-and-egg problem that occurs in the repository because (a) our Docker Compose files are stored in the same repository as our Dockerfiles and (b) we don't want the Docker Compose files used in our demos to require the user to go through long image build processes themselves.

The new CI/CD workflow will do the following:

  • Execute on every PR update, push to main, or push of a server tag.
  • Fail immediately if .env matches the version of any existing release.
  • Build and push Docker images on each PR change, with the contents of .env assigned as a shared version tag.
  • Push images to the EVerest/everest-demo GitHub Container Registry rather than the GHCR for US-JOET/everest-demo.

Layers to each of the Docker images built by this workflow are also cached to reduce repeat work and workflow execution times.

A future improvement would include a smoke test for each of the demos, validating that each demo's Docker Compose stack can be stood up with newly updated Docker images.

Rationale

Advantages:

  • Building and releasing new images is significantly simpler than our existing process.
  • The chicken-and-egg problem alluded to earlier is addressed completely. Docker Compose files can be updated to reference image versions created by the same changeset without making the Docker Compose files temporarily invalid while the new images are built.
    • The only exception to this will be this PR (and any hotfixes to this PR), since the new workflow will only kick in after we merge.
  • The images underlying the EVerest demos and the GitHub Action workflows that build them will now be hosted by the EVerest organization instead of US-JOET.
    Images with manual build steps (such as the SteVe-related images in Generate unique label in sphinx EVerest#7) can still be created outside of this deployment process without major headaches.
  • Modifications and additions to the demos can be made with only one PR/review cycle.

Disadvantages:

  • Contributors working in parallel can step on one another's image builds during development. This can become an issue when running Docker Compose files during development. Developers can avoid this issue by changing .env to include a branch-specific pre-release suffix like 1.2.3-pre-branchname.
    • This also means that squash-and-merge is the only merge strategy for the repository that will avoid pre-release image versions appearing in main.
  • The .env file is one more place developers must remember to update Docker image tags when changes are being made.
  • For us to continue developing on a public fork while executing workflows in EVerest/everest-demo, we may need @caller to provide @couryrr-afs and @drmrd write privileges for this repository (or provide add us as contributors to the EVerest org). (This is less of a problem and more of a nudge for us in a direction we're already headed.)

Tasks

  • Define a GitHub Action workflow for everest-demo that simplifies our existing build processes.
  • Update Docker image labels in all Dockerfiles and docker-compose.*.yml files to point to images in the EVerest GHCR namespace.
  • Document the new .env file and its usage in the README.md. I'll be working on this early next week.
  • [ ] Test the new workflow in US-JOET? I will work on this early next week. Given the lack of a functioning Docker image-building CI/CD pipeline for this repo, I propose merging changes here and addressing any issues in fast follow-up PRs to this repository if testing in US-JOET proves time-consuming. Unnecessary. See Simplify the current CI/CD workflow #10 (comment)

@@ -40,11 +54,9 @@ jobs:
tags: |
type=ref,event=branch
type=ref,event=pr
type=semver,pattern={{version}}
type=semver,pattern={{major}}.{{minor}}
type=semver,pattern=${{ steps.docker-image-version-check.SEMVER }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you articulate your reasons for preferring the manually updated semver to a more automatically generated unique tag based on date/time or timestamp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shankari: Sure! I've also added a description to this PR that covers my rationale. My main goal in making these changes is to

  1. get past issues we're experiencing with the build_pkg_*-based workflow so that we can merge the other Task 1 PRs in this repo and update our images
  2. while avoiding introducing the chicken-and-egg problem you had concerns over with our first pass CI/CD workflow (from our old private fork).
    • The chicken-and-egg problem I'm referring to: Our Docker Compose files reference images defined by Dockerfiles in the same repository. If our CI/CD pipeline only builds and pushes updated Docker images (with new semver tags) after we have merged changes into main, we either have to then update our Docker Compose files in a separate PR (which is time consuming and more overhead for devs and reviewers) or accept that our Docker Compose files will temporarily reference Docker images that don't exist (until the CI/CD pipeline finishes building and pushing them).

The advantage here is that we can set the tags for our Docker images manually to a new version as part of our PRs and have the updated images built before a PR is merged. When a PR is merged, the most recent image builds from the PR will be available either immediately or worst case (if another PR was using the same updated tag and clobbered this PR's builds in the Registry) within a few minutes.

Copy link
Contributor Author

@drmrd drmrd Dec 2, 2023

Choose a reason for hiding this comment

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

It's not intended to be an elegant solution long-term, but it gets us past our current build issues while also starting the process of moving these images into a more logical spot in the EVerest organization.

@shankari
Copy link
Collaborator

shankari commented Dec 4, 2023

For us to continue developing on a public fork while executing workflows in EVerest/everest-demo, we may need @caller to provide @couryrr-afs and @drmrd write privileges for this repository (or provide add us as contributors to the EVerest org). (This is less of a problem and more of a nudge for us in a direction we're already headed.)

@drmrd I don't think this is needed for building most of the images. The whole point of running this as a CI/CD workflow is that we can use the GitHub token and publish images without needing to expand the permission list. When we tested this on US-JOET, the images were built even you didn't have access.

Caveats:

  • We can still request org/write access for @drmrd and @couryrr-afs as part of the general rework of permissions as part of the working groups
  • The steve image is not currently built with the workflows, so we will need to come up with a solution on how that can be published. If we don't anticipate it being updated regularly, we can coordinate to build and push as a one-off.

@shankari
Copy link
Collaborator

shankari commented Dec 4, 2023

Test the new workflow in US-JOET? I will work on this early next week. Given the lack of a functioning Docker image-building CI/CD pipeline for this repo, I propose merging changes here and addressing any issues in fast follow-up PRs to this repository if testing in US-JOET proves time-consuming.

I requested, and have received, have write access to this repo temporarily while the working groups, and their permissions, are sorted out. Given that permission, I don't think we need to have this work on US-JOET. We are "upstream first" and the image builds on US-JOET were just a stopgap to deal with the lack of permission here.

Copy link
Collaborator

@shankari shankari left a comment

Choose a reason for hiding this comment

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

I'm approving the workflow anyway to see what happens 😄
But I expect it to fail, given that we are trying to build us-joet images through a workflow in the everest org.

.github/workflows/cicd.yaml Outdated Show resolved Hide resolved
docker-compose.admin-panel.yml Outdated Show resolved Hide resolved
@shankari
Copy link
Collaborator

shankari commented Dec 4, 2023

This also means that squash-and-merge is the only merge strategy for the repository that will avoid pre-release image versions appearing in main.

I'm not sure why squash-and-merge would make any difference. Concretely:

  • the GitHub action runs on every pull request to main (aka every update to a pull request)
  • if the developer has set the image version to 0.0.4-my-new-feature, then the action will go ahead and create an image with a tag 0.0.4-my-new-feature
  • presumably, before the final merge, developer will change and push the image version to 0.0.5, which will build the image and stomp any other 0.0.5s out there
  • we then finally merge (using any method) and the "push" part of the GitHub action will rewrite the 0.0.5 tag

It seems like the real issue is with pushing images on the pull request. What we really want is:

  • build images + future extension to run automated tests on PR
  • build + test + push images on branch push and/or tag/release creation

@drmrd
Copy link
Contributor Author

drmrd commented Dec 4, 2023

For us to continue developing on a public fork while executing workflows in EVerest/everest-demo, we may need @caller to provide @couryrr-afs and @drmrd write privileges for this repository (or provide add us as contributors to the EVerest org). (This is less of a problem and more of a nudge for us in a direction we're already headed.)

@drmrd I don't think this is needed for building most of the images. The whole point of running this as a CI/CD workflow is that we can use the GitHub token and publish images without needing to expand the permission list. When we tested this on US-JOET, the images were built even you didn't have access.

That would be good news. Whether or not PRs from public forks can automatically trigger GitHub Action workflows is a function of the repository and its org. It sounds like EVerest hasn't restricted workflow execution?

Caveats:

* We can still request org/write access for @drmrd and @couryrr-afs as part of the general rework of permissions as part of the working groups

👍 Having this happen as part of the working group permissions overhaul makes sense.

* The steve image is not currently built with the workflows, so we will need to come up with a solution on how that can be published. If we don't anticipate it being updated regularly, we can coordinate to build and push as a one-off.

Agreed. Once this workflow is in a good state, I'll update the OCPP 1.6J demo documentation with information on the new publishing process as part of that PR.

Copy link
Collaborator

@shankari shankari left a comment

Choose a reason for hiding this comment

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

LGTM! I am not going to hold up approval for this, but can you comment on
#10 (comment)
before I merge 😄

@drmrd
Copy link
Contributor Author

drmrd commented Dec 4, 2023

LGTM! I am not going to hold up approval for this, but can you comment on #10 (comment) before I merge 😄

Will do! Thanks @shankari!

@drmrd
Copy link
Contributor Author

drmrd commented Dec 5, 2023

This also means that squash-and-merge is the only merge strategy for the repository that will avoid pre-release image versions appearing in main.

I'm not sure why squash-and-merge would make any difference. Concretely:

* the GitHub action runs on every pull request to main (aka every update to a pull request)

* if the developer has set the image version to `0.0.4-my-new-feature`, then the action will go ahead and create an image with a tag `0.0.4-my-new-feature`

This is what I was referring to as a pre-release image. Using a rebase-and-merge strategy will result in a commit on the main branch that references an image tagged 0.0.4-my-new-feature, which isn't ideal.

It seems like the real issue is with pushing images on the pull request. What we really want is:

* build images + future extension to run automated tests on PR

* build + test + push images on branch push and/or tag/release creation

I agree this is the ideal. In our older private fork, we briefly had a pipeline set up like this before changing gears. I'm happy to remove the pushes on PRs here, but note that even with caching there can be a brief post-merge period during which Docker Compose files will point to images that don't exist in GHCR. Is this still a concern?

I'll plan to keep the push-on-PR portion of the workflow in tact for now but am happy to remove it later.

@drmrd drmrd marked this pull request as ready for review December 5, 2023 00:46
@drmrd drmrd force-pushed the simplify-cicd branch 3 times, most recently from b7a097c to f0f4548 Compare December 5, 2023 01:00
@drmrd
Copy link
Contributor Author

drmrd commented Dec 5, 2023

@caller: We're encountering a permissions issue when pushing images to GHCR from this repo's GitHub Actions workflow. Could you confirm that default permissions for the GITHUB_TOKEN in the EVerest organization are permissive (both read and write) as discussed here?

@drmrd
Copy link
Contributor Author

drmrd commented Dec 5, 2023

@shankari: Could you verify that this repo's workflows are allowed to write to GHCR as discussed here?

@shankari
Copy link
Collaborator

shankari commented Dec 5, 2023

@hikinggrass for visibility
@drmrd I have write permission on the repo, not admin. I don't see the settings.
Note however that we do have permissions in the workflow
https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs

jobs:
  docker-build-and-push-images:
    runs-on: ubuntu-latest
    permissions:
      contents: read
      packages: write

and that was good enough for us-joet

@shankari
Copy link
Collaborator

shankari commented Dec 5, 2023

I do note that you cannot grant write permissions to forked repos
https://docs.github.com/en/actions/security-guides/automatic-token-authentication#modifying-the-permissions-for-the-github_token

You can use the permissions key to add and remove read permissions for forked repositories, but typically you can't grant write access. The exception to this behavior is where an admin user has selected the Send write tokens to workflows from pull requests option in the GitHub Actions settings.

@shankari
Copy link
Collaborator

shankari commented Dec 5, 2023

I checked the us-joet settings, and we do have restrictive permissions for the repo

Screen Shot 2023-12-04 at 8 07 43 PM

and the org

Screen Shot 2023-12-04 at 8 10 44 PM

@shankari
Copy link
Collaborator

shankari commented Dec 5, 2023

@drmrd as a temporary workaround while waiting for the permission issues to be resolved, I suggest that we implement:

  • build images + future extension to run automated tests on PR
  • build + test + push images on branch push and/or tag/release creation

Note that secrets are available on push since it is approved code and not a pull request, which can potentially include malicious code.

Yes, "there can be a brief post-merge period during which Docker Compose files will point to images that don't exist in GHCR" but this is a stop-gap anyway and we can revisit what it will look like after all the permissions are finalized and all the images are built in the separate repos.

If we do end up keeping something like this for the long-term, there are workarounds we can consider (e.g. having two environment variables BUILD_TAG and DEPLOY_TAG and updating them at different times).

@drmrd
Copy link
Contributor Author

drmrd commented Dec 5, 2023

Thanks for digging into the permissions @shankari! I'll modify the workflow to not push during PRs.

@drmrd
Copy link
Contributor Author

drmrd commented Dec 5, 2023

@shankari: All changes are in. Please merge when you're available. 🚀

@shankari shankari merged commit af4eb3b into EVerest:main Dec 5, 2023
4 checks passed
@drmrd drmrd deleted the simplify-cicd branch December 5, 2023 22:31
@shankari
Copy link
Collaborator

shankari commented Dec 6, 2023

Filed #12 since the builds are not getting tagged properly

@drmrd
Copy link
Contributor Author

drmrd commented Dec 6, 2023

Looking into this now

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.

2 participants