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.

Sign up for GitHub

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

Jump to bottom

kbs: Build image on merge to main #170

Merged
merged 1 commit into from
Dec 2, 2023
Merged

kbs: Build image on merge to main #170

merged 1 commit into from
Dec 2, 2023

Conversation

kartikjoshi21
Copy link
Contributor

No description provided.

@kartikjoshi21 kartikjoshi21 force-pushed the kartikjoshi21/kbs-image branch 6 times, most recently from 21705eb to c6e1156 Compare October 19, 2023 11:03
@kartikjoshi21 kartikjoshi21 marked this pull request as ready for review October 19, 2023 11:04
@kartikjoshi21
Copy link
Contributor Author

@surajssd @mkulke @bpradipt can we create a repo in quay.io under confidential-containers repo to push these images on each merge to main ?

@mkulke
Copy link
Contributor

mkulke commented Oct 19, 2023

@surajssd @mkulke @bpradipt can we create a repo in quay.io under confidential-containers repo to push these images on each merge to main ?

why not ghcr? we wouldn't need credentials for those, I guess?

mkulke
mkulke reviewed Oct 19, 2023
View reviewed changes
.github/workflows/kbs_build_and_push.yaml Outdated Show resolved Hide resolved
.github/workflows/kbs_build_and_push.yaml Outdated
Comment on lines 34 to 36
DOCKER_BUILDKIT=1 docker build -t quay.io/confidential-containers/kbs:coco-as-${commit_sha} . -f docker/Dockerfile --push; \
DOCKER_BUILDKIT=1 docker build -t quay.io/confidential-containers/kbs:coco-as-openssl-${commit_sha} --build-arg KBS_FEATURES=coco-as-builtin,openssl,resource,opa . -f docker/Dockerfile --push; \
DOCKER_BUILDKIT=1 docker build -t quay.io/confidential-containers/kbs:coco-as-grpc-${commit_sha} . -f docker/Dockerfile.coco-as-grpc --push
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we limit it to a single configuration that is bundles most common features first?

- name Build and Push Container Image:
  run: DOCKER_BUILDKIT=1 docker build -t ghcr.io/confidential-containers/kbs-${{ github.sha }} --build-arg KBS_FEATURES=coco-as-builtin,openssl,resource,opa . -f docker/Dockerfile --push

@wainersm
Copy link
Member

@surajssd @mkulke @bpradipt can we create a repo in quay.io under confidential-containers repo to push these images on each merge to main ?

why not ghcr? we wouldn't need credentials for those, I guess?

I see that discussion of quay.io vs ghcr in other occasions. IIRC the last one we decided to stick with quay.io but at some point migrate everything to ghcr (which seems the preferable registry). For what I've seen ghcr has been used to deploy test/CI images, though.

@wainersm wainersm mentioned this pull request Oct 19, 2023
Open
@wainersm
Copy link
Member

@kartikjoshi21 how those images relates to https://github.com/confidential-containers/kbs/pkgs/container/key-broker-service ?

@kartikjoshi21 kartikjoshi21 force-pushed the kartikjoshi21/kbs-image branch 2 times, most recently from 211d262 to 019e73b Compare October 26, 2023 08:19
Xynnn007
Xynnn007 reviewed Oct 28, 2023
View reviewed changes
Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

Nice PR. Thanks for this. Let's discuss some details

.github/workflows/kbs_build_and_push.yaml Outdated
id: commit_sha
run: echo "::set-output name=sha::$(git rev-parse --short ${{ github.sha }})"

- name: Login to quay Container Registry
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- name: Login to quay Container Registry
- name: Login to GHCR Container Registry

.github/workflows/kbs_build_and_push.yaml Outdated
run: |
commit_sha=${{ steps.commit_sha.outputs.sha }}

DOCKER_BUILDKIT=1 docker build -t ghcr.io/kartikjoshi21/confidential-containers/kbs:kbs-${{ github.sha }} --build-arg KBS_FEATURES=coco-as-builtin,openssl,resource,opa . -f docker/Dockerfile --push
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can push the image to coco's community repo https://github.com/orgs/confidential-containers/packages.

cc @fitzthum wdyt?

Then we need to think about the location of the images. Two ideas from my side.

  1. The same repo as current kbs. The image name might be ghcr.io/confidential-containers/key-broker-service:commit-${{ github.sha }}
  2. A new repo for staged images (We can think that AS/RVPS would also need on every merge to main). The image name might be ghcr.io/confidential-containers/staged-images:kbs-commit-${{ github.sha }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe lets keep merge to main images as part of new repo to avoid confusion. What do you think ?

@kartikjoshi21 kartikjoshi21 mentioned this pull request Nov 2, 2023
Merged
@mkulke
Copy link
Contributor

mkulke commented Nov 7, 2023

@kartikjoshi21 at the moment this is not mergeable, right? it's pushing to your repo and that will only work on your fork (also, it's probably not what we want)

@kartikjoshi21 kartikjoshi21 force-pushed the kartikjoshi21/kbs-image branch 2 times, most recently from 2e61127 to 06ed40f Compare November 7, 2023 17:11
mkulke
mkulke requested changes Nov 8, 2023
View reviewed changes
.github/workflows/kbs_build_and_push.yaml Outdated Show resolved Hide resolved
.github/workflows/kbs_build_and_push.yaml Outdated Show resolved Hide resolved
@mythi mythi mentioned this pull request Nov 9, 2023
Closed
mkulke
mkulke reviewed Nov 10, 2023
View reviewed changes
.github/workflows/kbs_build_and_push.yaml Outdated
Comment on lines 19 to 24
- name: Login to GHCR Container Registry
uses: docker/login-action@v2
with:
registry: ghcr.io
username: ${{ secrets.repository_owner }}
password: ${{ secrets.GHCR_TOKEN }}
Copy link
Contributor

Choose a reason for hiding this comment

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

The auth scheme looks different from what's described here:

https://docs.github.com/en/actions/publishing-packages/publishing-docker-images#publishing-images-to-github-packages

Is GHCR_TOKEN something that is used as a convention elsewhere in the project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the naming convection per official document, Thanks

mkulke
mkulke approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@mkulke mkulke left a comment

Choose a reason for hiding this comment

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

lgmt. thanks

@Xynnn007
Copy link
Member

btw, the link error will be fixed in #216

Xynnn007
Xynnn007 approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

Thanks! lgtm

@Xynnn007
Copy link
Member

Seems that PR still needs some time to review. I put a quick fix PR #222

@Xynnn007
Copy link
Member

@kartikjoshi21 Would you please rebase the latest code? I think the link check can pass now.

@kartikjoshi21
Copy link
Contributor Author

@kartikjoshi21 Would you please rebase the latest code? I think the link check can pass now.

Thanks @Xynnn007.

@Xynnn007
Copy link
Member

Hi @wainersm please take a look if this pr looks good to you

@kartikjoshi21
Copy link
Contributor Author

@Xynnn007 @surajssd can we plan to merge it now ? Thanks

surajssd
surajssd approved these changes Dec 1, 2023
View reviewed changes
Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @kartikjoshi21

@Xynnn007 Xynnn007 merged commit 63d1963 into confidential-containers:main Dec 2, 2023
4 checks passed
@kartikjoshi21 kartikjoshi21 deleted the kartikjoshi21/kbs-image branch December 4, 2023 06:35
@Xynnn007
Copy link
Member

Xynnn007 commented Dec 4, 2023

@kartikjoshi21 Hi, seems that there is something wrong. Could you help for this? see https://github.com/confidential-containers/kbs/actions/runs/7083141332/job/19275029794

@portersrc
Copy link
Member

Maybe env.REGISTRY needs to be ghcr.io?

@surajssd
Copy link
Member

@Xynnn007 it is fixed by #254

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkulke mkulke mkulke approved these changes

@surajssd surajssd surajssd approved these changes

@Xynnn007 Xynnn007 Xynnn007 approved these changes

@sameo sameo Awaiting requested review from sameo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kartikjoshi21 @mkulke @wainersm @Xynnn007 @portersrc @surajssd