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

Run Agones tests in CI #551

Merged
merged 3 commits into from
Jul 29, 2022
Merged

Conversation

markmandel
Copy link
Contributor

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

What this PR does / Why we need it:

This makes it such that on each PR, the Agones integration tests are run
against the GKE + Agones cluster specified in ./agones/main.tf

This includes:

  • Fix for main.tf to align release branch with Agones version.
  • Configure how long to wait before cleaning up past test namespaces.
  • Cleanup and optimisation of cloudbuild.yaml.

Which issue(s) this PR fixes:

Work on #510

Special notes for your reviewer:

This makes it such that on each PR, the Agones integration tests are run
against the GKE + Agones cluster specified in ./agones/main.tf

This includes:
* Fix for main.tf to align release branch with Agones version.
* Configure how long to wait before cleaning up past test namespaces.
* Cleanup and optimisation of cloudbuild.yaml.

Work on googleforgames#510
@markmandel markmandel added kind/feature New feature or request area/tests Unit tests, integration tests, anything to make sure things don't break labels Jul 28, 2022
Copy link
Collaborator

@XAMPPRocky XAMPPRocky left a comment

Choose a reason for hiding this comment

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

LGTM, only nits

@@ -16,35 +16,34 @@ steps:
- name: gcr.io/cloud-builders/git
args: [ submodule, update, --init, --recursive ]
id: fetch-git-submodules
waitFor:
- "-"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a comment here explaining this. Maybe this is obvious to people who use cloudbuild, but it's not obvious to me compared to other usages of waitFor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent suggestion - will do.

Comment on lines 82 to 83
let delay = env::var_os(DELETE_DELAY_SECONDS)
.map(|value| chrono::Duration::seconds(value.into_string().unwrap().parse().unwrap()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: You don't need to use var_os if you're converting to string anyway, and you can flatten the unwraps.

Suggested change
let delay = env::var_os(DELETE_DELAY_SECONDS)
.map(|value| chrono::Duration::seconds(value.into_string().unwrap().parse().unwrap()));
let delay = env::var(DELETE_DELAY_SECONDS)
.as_deref()
.ok()
.and_then(|s| i64::parse(s).ok())
.map(chrono::Duration::seconds);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I swear I never saw env::var() existing before. Totally missed that. Thank you!

@markmandel
Copy link
Contributor Author

GCP IAM permission on fail (which I expected but wasn't sure if we would hit).

@markmandel
Copy link
Contributor Author

markmandel commented Jul 29, 2022

That took an hour - I should look at why. I think some of my optimisations in cloudbuild.yaml caused a bunch of redundant work to be done.

I swore I tested this locally first, but I'll triple check things before merging.

@markmandel
Copy link
Contributor Author

Ah! Found it.

@quilkin-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 104f697e-da23-4050-95e6-fcdb4ddcd216

To build this version:

git fetch git@github.com:googleforgames/quilkin.git pull/551/head:pr_551 && git checkout pr_551
cargo build

@markmandel markmandel merged commit 9e33f1e into googleforgames:main Jul 29, 2022
@markmandel markmandel deleted the wip/agones-ci branch July 29, 2022 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tests Unit tests, integration tests, anything to make sure things don't break kind/feature New feature or request size/s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants