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.

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

Merged
merged 7 commits into from Dec 4, 2023

Conversation

Kalaiselvi84
Copy link
Contributor

@Kalaiselvi84 Kalaiselvi84 commented Nov 29, 2023

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug
/kind cleanup
/kind documentation
/kind feature
/kind hotfix
/kind release

What this PR does / Why we need it:

Which issue(s) this PR fixes:

Closes #3476

Special notes for your reviewer:

@Kalaiselvi84
Copy link
Contributor Author

@markmandel, Could you please review this PR? I am unsure if I am in the right direction.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 1ba134d9-cd71-408b-9611-5f108ec5c1e4

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

Copy link
Member

@markmandel markmandel left a 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?

@google-oss-prow google-oss-prow bot added size/XL and removed size/L labels Nov 29, 2023
@Kalaiselvi84
Copy link
Contributor Author

I assume this is a vendoring of the code from the community repo? But looks like you updated the tags to use Artifact Registry? 👍🏻

Yes, that's correct. However, I am fine with the second approach. Looking forward to hearing your preference.

This should sit somewhere in /build -- maybe under /build/build-image/cache ?

Moved files to /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.

Done!

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.

Included the origin and the changes made in our repo.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 654c4256-b735-48ff-b120-4d0cfeda18d9

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

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:

  • git fetch https://github.com/googleforgames/agones.git pull/3522/head:pr_3522 && git checkout pr_3522
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.37.0-dev-96d748e-amd64

@Kalaiselvi84
Copy link
Contributor Author

The cloudbuild.yaml script is failing; it looks like we need to modify the steps based on our repo.🤔

@markmandel
Copy link
Member

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?

@Kalaiselvi84
Copy link
Contributor Author

What's the output when it fails?

Log indicates: Step #1 - "build_save_cache": Unknown option: build
Here's the full log - https://gist.github.com/Kalaiselvi84/13b9286ad4ab6e1e0477d79eeb06c96c

Copy link
Member

@markmandel markmandel left a 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'
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: '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'
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: 'us-docker.pkg.dev/$PROJECT_ID/ci/restore_cache'
- name: 'gcr.io/cloud-builders/docker'

Copy link
Contributor Author

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.😬

@agones-bot
Copy link
Collaborator

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:

  • git fetch https://github.com/googleforgames/agones.git pull/3522/head:pr_3522 && git checkout pr_3522
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.37.0-dev-cf166d0-amd64

Copy link
Member

@markmandel markmandel left a 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.

@@ -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:
Copy link
Member

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.

Copy link
Contributor Author

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.

@Kalaiselvi84 Kalaiselvi84 marked this pull request as ready for review December 1, 2023 21:58
@Kalaiselvi84 Kalaiselvi84 changed the title ci:save_cache and restore_cache Ensure ci/save_cache and ci/restore_cache images don't get deleted by cleanup policy Dec 1, 2023
Copy link

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: fa08d1e7-23f5-4cec-a6b9-05d37fea590b

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

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:

  • git fetch https://github.com/googleforgames/agones.git pull/3522/head:pr_3522 && git checkout pr_3522
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.37.0-dev-8249e5c-amd64

@markmandel markmandel merged commit 5992e2a into googleforgames:main Dec 4, 2023
3 checks passed
@Kalaiselvi84 Kalaiselvi84 added area/tests Unit tests, e2e tests, anything to make sure things don't break area/build-tools Development tooling. I.e. pretty much everything in the `build` directory. and removed kind/other labels Dec 12, 2023
@Kalaiselvi84 Kalaiselvi84 deleted the cache branch March 15, 2024 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/build-tools Development tooling. I.e. pretty much everything in the `build` directory. area/tests Unit tests, e2e tests, anything to make sure things don't break lgtm size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: Ensure ci/save_cache and ci/restore_cache images don't get deleted by cleanup policy
3 participants