[GSC] Adding Github action to build Graphene Docker image #1857
Conversation
vahldiek
changed the title
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @vahldiek)
a discussion (no related file):
The commit message must be expanded to include the bulk of the PR description. I'd like to have an explanation of what happens and why while browsing commit history.
a discussion (no related file):
What is "PW" in your PR description? PW == Password?
.github/workflows/graphene-base-image.yaml, line 21 at r1 (raw file):
run: | pip install jinja2 pyyaml docker - name: Build the Grahene Docker image
Graphene
.github/workflows/graphene-base-image.yaml, line 28 at r1 (raw file):
echo "$DOCKER_PASSWORD" | docker login -u "$DOCKER_USERNAME" --password-stdin cd Tools/gsc mkdir images
I would prefer mkdir -p images
Jenkinsfiles/Linux-SGX-gsc, line 24 at r1 (raw file):
sh ''' cd Tools/gsc make build-images
Is this just to test that prebuilt Docker images are built correctly? I.e., that a PR doesn't break the AKS-Graphene Docker image?
Tools/gsc/Makefile, line 10 at r1 (raw file):
IMAGES=aks VERSIONS=latest REPOSITORY_NAME=graphenelibos
Is this the official Docker Hub repository name? We should have a comment about this here (like if we decide to change the name in Docker Hub, we should also change this one).
Tools/gsc/Makefile, line 52 at r1 (raw file):
.PHONY: clean clean: $(RM) config.aks.*.yaml
I don't see how we can call clean target now. Was it intended?
vahldiek
force-pushed
the
vahldiek/workflow-docker-image
branch
2 times, most recently
from
9d2af0a
to
2ec1c8f
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 4 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The commit message must be expanded to include the bulk of the PR description. I'd like to have an explanation of what happens and why while browsing commit history.
I've tried my best to include more info...
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What is "PW" in your PR description? PW == Password?
Yes, changed the PR description
.github/workflows/graphene-base-image.yaml, line 21 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Graphene
Done.
.github/workflows/graphene-base-image.yaml, line 28 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would prefer
mkdir -p images
Done.
Jenkinsfiles/Linux-SGX-gsc, line 24 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Is this just to test that prebuilt Docker images are built correctly? I.e., that a PR doesn't break the AKS-Graphene Docker image?
Yes. This is just to test the build. I'll add a comment.
Tools/gsc/Makefile, line 10 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Is this the official Docker Hub repository name? We should have a comment about this here (like if we decide to change the name in Docker Hub, we should also change this one).
Yes, this is the current organization name. If we change the organization name, we have to change it here once. Added a comment.
Tools/gsc/Makefile, line 52 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't see how we can call
cleantarget now. Was it intended?
Done. Added it back to distclean.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @vahldiek)
.github/workflows/graphene-base-image.yaml, line 28 at r1 (raw file):
Previously, vahldiek (Anjo Vahldiek-Oberwagner) wrote…
Done.
You moved it to the Makefile, but see my comment there. I think it will break.
Tools/gsc/Makefile, line 10 at r2 (raw file):
IMAGES=aks VERSIONS=latest # Official Docker Hub organization name. In case of a name change in Docker Hub, this name as to be
as to be -> must be
Tools/gsc/Makefile, line 41 at r2 (raw file):
.PHONY: build-aks-% build-aks-%: images/graphene_aks.%.dockerfile
Shouldn't you have images as a dependency here? It looks like you may call build-images without this directory created, and everything will break.
There was a problem hiding this comment.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
Tools/gsc/Makefile, line 10 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
as to be->must be
Done.
Tools/gsc/Makefile, line 41 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Shouldn't you have
imagesas a dependency here? It looks like you may callbuild-imageswithout this directory created, and everything will break.
Yes, good catch. I place the dependency of images where it is actually needed (when generating the files in images/).
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @vahldiek)
Tools/gsc/Makefile, line 10 at r2 (raw file):
Previously, vahldiek (Anjo Vahldiek-Oberwagner) wrote…
Done.
Not done. must be changed or must change, but not must changed.
vahldiek
force-pushed
the
vahldiek/workflow-docker-image
branch
from
902b639
to
753ad29
Compare
There was a problem hiding this comment.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv)
Tools/gsc/Makefile, line 10 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not done.
must be changedormust change, but notmust changed.
Sorry
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
vahldiek
force-pushed
the
vahldiek/workflow-docker-image
branch
from
753ad29
to
4ba93b6
Compare
There was a problem hiding this comment.
I rebased the changes to the most recent master, which moved Jenkins files to .ci.
Reviewable status: 3 of 4 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: )
|
Jenkins, test this please |
There was a problem hiding this comment.
Reviewed 1 of 2 files at r5.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
There was a problem hiding this comment.
Please keep in mind that we have to set the secrets for the dockerhub user and password in the Github settings. Ping me on Slack if they are missing. Thanks!
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
vahldiek
force-pushed
the
vahldiek/workflow-docker-image
branch
from
4ba93b6
to
8538e67
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
…er image. Add Github action that logs into Docker Hub via Github Secrets, builds the Graphene base Docker image (currently only for AKS), and pushes the resulting image to Docker Hub. Signed-off-by: Anjo Vahldiek-Oberwagner <[email protected]>
vahldiek
force-pushed
the
vahldiek/workflow-docker-image
branch
2 times, most recently
from
561df84
to
d24242a
Compare
There was a problem hiding this comment.
Also got around to rebase this PR. Please remember that this PR needs the Dockerhub credentials in the Github Secrets of the Graphene project. Otherwise the action will always fail.
Reviewable status: 3 of 4 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv)
There was a problem hiding this comment.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @vahldiek)
a discussion (no related file):
Why do we want/need this functionality in Graphene? Do we want to provide an up-to-date Graphene Docker image for AKS? I don't remember the context to introduce this... Is this high priority?
There was a problem hiding this comment.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do we want/need this functionality in Graphene? Do we want to provide an up-to-date Graphene Docker image for AKS? I don't remember the context to introduce this... Is this high priority?
Not high priority, but then I would make the public Graphene Dockerhub repository private, since I don't want to maintain it manually any longer.
Generally, the suggestion was to provide a known base image for popular cloud providers. AKS being on of them.
Description of the changes
This PR adds a continuous deployment action building the base Graphene Docker image for Azure Kubernetes Services via Github actions. The action is implemented in
.github/workflows/graphene-base-image.yaml. It is only triggered on pushes to the master branch and only for the main Graphene repositoryoscarlab/grapheneand not for any fork. Even if someone changes the second check, they cannot not overwrite the public Dockerhub images, since they're missing the login credentials to the Dockerhub account.The action logs into Docker Hub via Github Secrets (which have to be defined in the Graphene Repo setting) while ensuring that the password does not appear in public output logs, builds the AKS image, and pushes the resulting image to Docker Hub.
This PR also includes additional changes to test the scripts via Jenkins and removes the previously added Dockerfiles under
Tools/gsc/images, since they're now generated during the Github action.How to test this PR?
Run GSC Jenkins test (this tests the build of the base Graphene image), but does not test the actual Github action and the Docker Hub login. There is no way to test this before hand. You can find a successful workflow of my private repository here: https://github.com/vahldiek/graphene/actions/runs/280306800/workflow This action tough used a different branch and repository check. (otherwise the workflow is exactly the same as committed here)
This change is