Closed drmrd closed 7 months ago
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:
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.
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:
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
It seems like the real issue is with pushing images on the pull request. What we really want is:
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.
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!
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.
@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?
@shankari: Could you verify that this repo's workflows are allowed to write to GHCR as discussed here?
@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
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.
I checked the us-joet settings, and we do have restrictive permissions for the repo
and the org
@drmrd as a temporary workaround while waiting for the permission issues to be resolved, I suggest that we implement:
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).
Thanks for digging into the permissions @shankari! I'll modify the workflow to not push during PRs.
@shankari: All changes are in. Please merge when you're available. 🚀
Filed https://github.com/EVerest/everest-demo/issues/12 since the builds are not getting tagged properly
Looking into this now
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.v0.0.3
, the.env
will contain0.0.3
.v0.0.4
,.env
should contain0.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 fromeverest-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:
main,
or push of a server tag..env
matches the version of any existing release..env
assigned as a shared version tag.EVerest/everest-demo
GitHub Container Registry rather than the GHCR forUS-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:
Disadvantages:
.env
to include a branch-specific pre-release suffix like1.2.3-pre-branchname
.main
..env
file is one more place developers must remember to update Docker image tags when changes are being made.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
everest-demo
that simplifies our existing build processes.Dockerfile
s anddocker-compose.*.yml
files to point to images in the EVerest GHCR namespace..env
file and its usage in theREADME.md
. I'll be working on this early next week.