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

ref(workflows): use a single job to run GCP tests #7682

Merged
merged 53 commits into from
Oct 19, 2023
Merged

Conversation

gustavovalverde
Copy link
Member

Motivation

To be able to handle edge-cases and complex scenarios (launch step no failing, but test-result failint and not being able to re-run it), and for the sake of simplicity, this PR proposes using a single job for handling all the following scenarios:

  • Test without cached states
  • Test with Zebra cached state
  • Test with Lightwalletd cached state

Complex Code or Requirements

This is a very delicate change, even though is simplifying the process, as it has to consider multiple scenarios which were using multiple jobs and steps, and now it's using a single job and some steps have been merged.

Solution

  • Use variables like $DISK_OPTION and $MOUNT_FLAGS to allow flexibility when mounting the volumes.

Review

  • We should run most tests across this changes

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

Move the scripts out of the workflow.

@gustavovalverde gustavovalverde requested a review from a team as a code owner October 5, 2023 16:16
@gustavovalverde gustavovalverde requested review from oxarbitrage and removed request for a team October 5, 2023 16:16
@gustavovalverde gustavovalverde self-assigned this Oct 5, 2023
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 5, 2023
@gustavovalverde gustavovalverde added A-infrastructure Area: Infrastructure changes A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement C-cleanup Category: This is a cleanup P-High 🔥 I-usability Zebra is hard to understand or use and removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Oct 5, 2023
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This looks good! Thank you for this cleanup.

Is there a description of how the workflow works now? Can we add that to the top of this workflow?

It looks like you merged all 3 of the cases in the PR description into the same job, but also made some other changes to simplify pipe handling. Is there any other significant change here?

.github/workflows/ci-integration-tests-gcp.yml Outdated Show resolved Hide resolved
.github/workflows/sub-deploy-integration-tests-gcp.yml Outdated Show resolved Hide resolved
.github/workflows/sub-deploy-integration-tests-gcp.yml Outdated Show resolved Hide resolved
.github/workflows/sub-deploy-integration-tests-gcp.yml Outdated Show resolved Hide resolved
@upbqdn upbqdn removed the do-not-merge Tells Mergify not to merge this PR label Oct 17, 2023
Base automatically changed from refactor-workflows to main October 18, 2023 06:16
@mergify mergify bot requested a review from a team as a code owner October 18, 2023 06:16
@teor2345
Copy link
Contributor

I removed the Run lwd-full-sync-test branch protection rule because it very rarely runs in PRs, it runs in a scheduled job now.

@teor2345
Copy link
Contributor

@gustavovalverde I just saw this $DISK_NAME fix follow-up again in my final review:
https://github.com/ZcashFoundation/zebra/pull/7682/files#r1357377327

It would be ok to copy the same script you use to find the disk name into that SSH command.

mergify bot added a commit that referenced this pull request Oct 19, 2023
mergify bot added a commit that referenced this pull request Oct 19, 2023
mergify bot added a commit that referenced this pull request Oct 19, 2023
mergify bot added a commit that referenced this pull request Oct 19, 2023
@mergify mergify bot merged commit d6f4d31 into main Oct 19, 2023
129 of 130 checks passed
@mergify mergify bot deleted the ref-dry-workflows branch October 19, 2023 11:52
mergify bot added a commit that referenced this pull request Oct 19, 2023
@teor2345 teor2345 mentioned this pull request Nov 5, 2023
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles A-infrastructure Area: Infrastructure changes C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-usability Zebra is hard to understand or use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants