Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Move all actual Agones releases images to GAR #2875
Changes from 5 commits
ac8a751
f6f6f02
90d744b
deee464
5f0d4c7
2a4dc9c
4578006
5767779
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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!
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 useREGISTRY
as part of the image tag. Currently it's stillgcr.io/agones-images
(Though I haven't figured out where this value come from). I think theagones-build
image will be in theus-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
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.yamlI'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 theREGISTRY
come from in this file. So if we do not specify theREGISTRY
env viable in thesite/cloud-build.yaml
, will it try to pull the image fromgcr.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:agones/build/Makefile
Line 40 in 176ffb2
So in your code, the
REGISTRY
variable is removed from the parentMakefile
, 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
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 theus-docker.pkg.dev/${PROJECT_ID}/ci
, and the correspondingagones-build
image exists inus-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!