-
Notifications
You must be signed in to change notification settings - Fork 813
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
Move all actual Agones releases images to GAR #2875
Conversation
Build Failed 😱 Build Id: 26d76fff-f2e7-4137-863a-6e91a35b2352 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: c983af35-48d0-4957-8601-66a09d9b1d3b The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 4754c549-6065-47e6-821d-9ec14d95752e The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Failed 😱 Build Id: 4cbcdc28-2f6e-4363-a42b-2cb75155c110 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 4f16b658-001f-43c4-9372-32aaa38c3987 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: e4ff6612-f783-407c-bc6e-6cd22bddf1a4 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 099a213d-94d8-4440-9398-4e93835322ad The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 3ef5903a-9a7d-44c6-983d-9c8a3d0fb705 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 186d6d16-fd50-4e1e-94cf-dce795bf2b0f The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
I'll take an extra look at this today - primarily I want to double check the release makefile commands and checklist 👍🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - but would love confirmation on the change to site/cloudbuild.yaml before merging.
gcloud-auth-docker: $(ensure-build-image) | ||
docker run --rm $(common_mounts) $(build_tag) gcloud auth print-access-token | docker login -u oauth2accesstoken --password-stdin https://gcr.io | ||
docker run --rm $(common_mounts) $(build_tag) gcloud auth print-access-token | docker login -u oauth2accesstoken --password-stdin https://us-docker.pkg.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I forgot that I kept having to do this manually!
@@ -29,6 +29,8 @@ steps: | |||
args: ['build', '-f', 'Dockerfile.build', '-t', 'make-docker', '.'] # we need docker and make to run everything. | |||
- name: "make-docker" | |||
dir: "build" | |||
env: | |||
- "REGISTRY=${_REGISTRY}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 what does this do? I don't think this is required, but I could be wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused now. This step will do pull-build-image
which use REGISTRY
as part of the image tag. Currently it's still gcr.io/agones-images
(Though I haven't figured out where this value come from). I think the agones-build
image will be in the us-docker.pkg.dev/agones-images/ci
(this change)? Though the change I made here will not work either, since I didn't specify _REGISTRY
in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this all comes from here:
agones/build/includes/build-image.mk
Line 51 in 176ffb2
pull-build-image: |
So pull and push build image targets are targets specifically for the CI system (local developers don't use them), so for those, I don't know if you want to set them to a default that will work on CI?
Also, if you have _REGISTRY
you will need to specify it in the substitutions block of this cloudbuild.yaml
I'm curious if you tested running this cloud-build yaml against your dev project to see if it worked - because I would have expected it would have failed because of the missing _REGISTRY
definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I understood they come from the build-image.mk
. I'm just not sure where the REGISTRY
come from in this file. So if we do not specify the REGISTRY
env viable in the site/cloud-build.yaml
, will it try to pull the image from gcr.io
?
You're right the _REGISTRY
definition is missed. I only noticed it when I saw your last comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OIC OIC! So the REGISTRY
variable in the Makefile could be set in the parent Makefile or any includes that came before it.
So in the main
version of the code, it come from here:
Line 40 in 176ffb2
REGISTRY ?= $(release_registry) |
So if we do not specify the REGISTRY env viable in the site/cloud-build.yaml, will it try to pull the image from gcr.io?
So in your code, the REGISTRY
variable is removed from the parent Makefile
, so I'm not sure where it would be getting it from -- I actually expect that it would fail silently, since there wouldn't be a registry attached.
agones/build/includes/build-image.mk
Lines 70 to 72 in 176ffb2
# pull a local image that may exist on a remote repository, to a local image | |
pull-remote-build-image: | |
-docker pull $(REMOTE_TAG) && docker tag $(REMOTE_TAG) $(LOCAL_TAG) |
So TL;DR - I would suggest either hardcode where it's pulled from somewhere in that Makefile, or create a substitution section like we have in the main cloudbuild.yaml and pull it from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the details! This makes sense now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the substitution in site/cloudbuild.yaml
. Confirmed the cloud build step is looking at the us-docker.pkg.dev/${PROJECT_ID}/ci
, and the corresponding agones-build
image exists in us-docker.pkg.dev/agones-images/ci
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks right! Okay, let's approve and hopefully nothing breaks 😀
But looks good to me!
Build Succeeded 👏 Build Id: 39f03b6f-b560-46ab-b7b3-5b3d993de151 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gongmax, markmandel, roberthbailey The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #2358
Special notes for your reviewer:
I created the registry
us-docker.pkg.dev/agones-images/release
manually.Tested by following the instructions to run a test GKE cluster