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.

Sign up for GitHub

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

Jump to bottom

ci(sync): only run the lightwalletd full sync on the main branch #5393

Merged
merged 4 commits into from
Oct 19, 2022
Merged

ci(sync): only run the lightwalletd full sync on the main branch #5393

merged 4 commits into from
Oct 19, 2022

Conversation

gustavovalverde
Copy link
Member

@gustavovalverde gustavovalverde commented Oct 12, 2022 •
edited by teor2345
Loading

Previous behavior

In PR #5164, we made lightwalletd sync all the way to the tip in its full sync test.

This increases that test's time from 1 hour to 4 hours, which makes the CI we run on each PR change increase from 3 hours to 6 hours.

Expected behavior

Run the lightwalletd full sync just on main or if a state disk for the actual version is not found.

Solution

Add the github.event_name == 'push' && github.ref_name == 'main' condition to the lightwalletd-full-sync test.

Fixes #5316

Review

Anyone from the DevOps team
How to test: This job should not run in this PR

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?
  • How do you know it works? Does it have tests?

Follow Up Work

@gustavovalverde
ci(sync): only run the lightwalletd full sync on the main branch
c6c621c 
Previous behavior:
In PR #5164, we made lightwalletd sync all the way to the tip in its full
sync test.

This increases that test's time from 1 hour to 4 hours, which makes the CI
we run on each PR change increase from 3 hours to 6 hours.

Expected behavior:
Run the lightwalletd full sync just on `main` or if a state disk for the
actual version is not found.

Solution:
Add the `github.event_name == 'push' && github.ref_name == 'main'` condition
to the `lightwalletd-full-sync` test.

Fixes #5316
@gustavovalverde gustavovalverde added A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement P-High 🔥 I-usability Zebra is hard to understand or use labels Oct 12, 2022
@gustavovalverde gustavovalverde requested a review from a team as a code owner October 12, 2022 16:40
@gustavovalverde gustavovalverde self-assigned this Oct 12, 2022
@gustavovalverde gustavovalverde requested review from arya2 and removed request for a team October 12, 2022 16:40
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 12, 2022
arya2
arya2 previously approved these changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gustavovalverde
Copy link
Member Author

Protection rules updated to allow this and other PRs to be merged

teor2345
teor2345 suggested changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Protection rules updated to allow this and other PRs to be merged

This PR disables the requirement for the lwd full sync job, but we added a patch job for the zebrad full sync job.

Screen Shot 2022-10-13 at 07 25 07

zebra/.github/workflows/continous-integration-docker.patch-always.yml

Lines 18 to 22 in 8646a03

test-full-sync:
name: Zebra tip / Run full-sync-to-tip test
runs-on: ubuntu-latest
steps:
- run: 'echo "No build required"'

Can we choose one way to do it, and make it consistent?

teor2345
teor2345 suggested changes Oct 12, 2022
View reviewed changes
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.

I'm not sure about the conditions here, I think we might want to match the full sync job almost exactly?

I'll also push a PR with some more suggestions.

This was referenced Oct 12, 2022
Merged
Merged
@teor2345
Copy link
Contributor

I opened PRs #5399 and #5400, feel free to merge them into this PR, or merge them into main separately.

PR #5400 will conflict with my job condition suggestion above, because it adds to that same line.

@gustavovalverde
Copy link
Member Author

This has been updated accordingly

@teor2345 teor2345 mentioned this pull request Oct 19, 2022
Closed
@teor2345
Copy link
Contributor

Protection rules updated to allow this and other PRs to be merged

This PR disables the requirement for the lwd full sync job, but we added a patch job for the zebrad full sync job.

I added an "always" patch job, and re-added the branch protection rule.

teor2345
teor2345 approved these changes Oct 19, 2022
View reviewed changes
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.

Thanks, I think we're good to go here.

mergify bot added a commit that referenced this pull request Oct 19, 2022
@mergify
Merge of #5393
8ffb815
@mergify mergify bot mentioned this pull request Oct 19, 2022
Closed
@mergify mergify bot merged commit dae1d92 into main Oct 19, 2022
@mergify mergify bot deleted the move-full-lwd branch October 19, 2022 12:01
@teor2345 teor2345 mentioned this pull request Oct 26, 2022
Merged
5 tasks
@teor2345 teor2345 mentioned this pull request Nov 1, 2022
Merged
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

@arya2 arya2 arya2 left review comments

Assignees

@gustavovalverde gustavovalverde

Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Only run the lightwalletd full sync on the main branch
3 participants
@gustavovalverde @teor2345 @arya2