-
Notifications
You must be signed in to change notification settings - Fork 826
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
Ensure ci/save_cache and ci/restore_cache images don't get deleted by cleanup policy #3522
Conversation
@markmandel, Could you please review this PR? I am unsure if I am in the right direction. |
Build Failed 😱 Build Id: 1ba134d9-cd71-408b-9611-5f108ec5c1e4 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
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 assume this is a vendoring of the code from the community repo? But looks like you updated the tags to use Artifact Registry? 👍🏻
This should sit somewhere in /build
-- maybe under /build/build-image/cache
?
The only other thing that looks like something we should do is ensure each file has the Apache 2 licence. It's weird that the original builder files don't.... but we should definitely make sure we do.
Ooh - also, let's throw a README.md in the directory explaining where this originally came from and what changes were made on import in case we ever want to update it.
Sound good?
Yes, that's correct. However, I am fine with the second approach. Looking forward to hearing your preference.
Moved files to
Done!
Included the origin and the changes made in our repo. |
Build Failed 😱 Build Id: 654c4256-b735-48ff-b120-4d0cfeda18d9 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: e321f0b5-928c-4683-873b-bd558af67517 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:
|
The cloudbuild.yaml script is failing; it looks like we need to modify the steps based on our repo.🤔 |
What's the output when it fails? |
Log indicates: Step #1 - "build_save_cache": Unknown option: build |
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.
Try that 👍🏻
- '--file=Dockerfile-base' | ||
- '.' | ||
|
||
- name: 'us-docker.pkg.dev/$PROJECT_ID/ci/save_cache' |
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.
- name: 'us-docker.pkg.dev/$PROJECT_ID/ci/save_cache' | |
- name: 'gcr.io/cloud-builders/docker' |
Since this is building the docker container, so it needs to use docker as the build step container.
- '--cache-from=us-docker.pkg.dev/$PROJECT_ID/ci/cache:latest' | ||
- '.' | ||
|
||
- name: 'us-docker.pkg.dev/$PROJECT_ID/ci/restore_cache' |
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.
- name: 'us-docker.pkg.dev/$PROJECT_ID/ci/restore_cache' | |
- name: 'gcr.io/cloud-builders/docker' |
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.
Success Log: https://gist.github.com/Kalaiselvi84/5ccadb47f469435391ce91a3148cfeae
Whole time I was searching for the alternate options.😬
Build Succeeded 👏 Build Id: 07a4e632-8a03-4c41-9b43-7993139f8bf6 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:
|
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 - one comment and this can definitely come out of draft.
build/includes/release.mk
Outdated
@@ -87,3 +87,8 @@ pre-build-release: | |||
post-build-release: | |||
docker run --rm $(common_mounts) -w $(workdir_path)/build/release $(build_tag) \ | |||
gcloud builds submit . --substitutions _VERSION=$(base_version) --config=./post_cloudbuild.yaml $(ARGS) | |||
|
|||
# Ensure that ci/save_cache and ci/restore_cache are not deleted by the cleanup policy | |||
cache-save-restore: |
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 don't need this for the setup of the Cloud Build scheduled triggers, but if we want to keep it for testing purposes (which is useful), I think it might be better to be part of https://github.com/googleforgames/agones/blob/main/build/includes/build-image.mk since it's not a target needed for the release process.
Open to suggestion on a better place, but that's the best idea I had.
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.
Understood. I'll go ahead and remove it since it's not required for the setup of the Cloud Build scheduled triggers.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Kalaiselvi84, markmandel 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 |
Build Failed 😱 Build Id: fa08d1e7-23f5-4cec-a6b9-05d37fea590b To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 480aff36-7247-45ff-a88f-41bf0704b3e2 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:
|
What type of PR is this?
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #3476
Special notes for your reviewer: