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

change(ci): only run the send transaction test on the main branch #5480

Merged
merged 5 commits into from
Oct 26, 2022
Merged

change(ci): only run the send transaction test on the main branch #5480

merged 5 commits into from
Oct 26, 2022

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Oct 25, 2022 •
edited by teor2345
Loading

Motivation

The send transaction test is slow, so we want to move it to the main branch until we've made the changes in #5015

Closes #5426

Review

This PR is part of regular scheduled work.

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

Revert this change once #5015 has been addressed.

@arya2 arya2 added A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement P-Medium ⚡ I-slow Problems with performance or responsiveness C-testing Category: These are tests I-cost Zebra infrastructure costs labels Oct 25, 2022
@arya2 arya2 requested a review from a team as a code owner October 25, 2022 17:28
@arya2 arya2 requested review from dconnolly and removed request for a team October 25, 2022 17:28
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 25, 2022
@arya2 arya2 requested review from gustavovalverde and removed request for dconnolly October 25, 2022 17:31
@arya2
Copy link
Contributor Author

arya2 commented Oct 25, 2022 •
edited
Loading

Should this PR allow the send transaction test to be triggered manually?

teor2345
teor2345 suggested changes Oct 26, 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.

Just a few minor details to fix up so we satisfy the branch protection rules.

Do you think we will ever want to run this job manually?

I'm not sure, because we're not actually generating a cached state with this job. So the only reason to run it manually would be if we updated the code. (Which we might do in #5015, or we might just leave it.)

Here's what we did to add a manual full lightwalletd sync:
https://github.com/ZcashFoundation/zebra/pull/5393/files#diff-4dc8170bbb198e5ddb8c8d186aa2cf20ee5117960f71ad9197b2a800ad878bceR31-R35

.github/workflows/continous-integration-docker.patch-always.yml Outdated Show resolved Hide resolved
.github/workflows/continous-integration-docker.yml Outdated Show resolved Hide resolved
.github/workflows/continous-integration-docker.yml Show resolved Hide resolved
@arya2 @teor2345
Apply suggestions from code review
26e9e0d 
Co-authored-by: teor <teor@riseup.net>
@arya2
Copy link
Contributor Author

arya2 commented Oct 26, 2022

Do you think we will ever want to run this job manually?

I don't see any harm in making it available for testing #5015.

@arya2 arya2 force-pushed the move-send-transactions-test branch from edbee35 to b2048fc Compare October 26, 2022 16:57
@arya2 arya2 requested a review from teor2345 October 26, 2022 17:00
teor2345
teor2345 approved these changes Oct 26, 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, looks good!

mergify bot added a commit that referenced this pull request Oct 26, 2022
@mergify
Merge of #5480
ffd8ddf
@mergify mergify bot mentioned this pull request Oct 26, 2022
Closed
@mergify mergify bot merged commit 466adc7 into main Oct 26, 2022
@mergify mergify bot deleted the move-send-transactions-test branch October 26, 2022 21:26
@teor2345 teor2345 mentioned this pull request Nov 1, 2022
Merged
33 tasks
@arya2 arya2 mentioned this pull request Nov 9, 2022
Closed
gustavovalverde added a commit that referenced this pull request Jan 27, 2023
@gustavovalverde
ci(lwd): run the send transactions test on each PR update
6f6c5b6 
The send transactions test was moved to the main branch in #5480 because
it was very slow.

It's much faster (~30m) with #5015 and now it can be run for every PR
update again.
@gustavovalverde gustavovalverde mentioned this pull request Jan 27, 2023
Merged
6 tasks
mergify bot pushed a commit that referenced this pull request Jan 31, 2023
@gustavovalverde
ci(lwd): run the send transactions test on each PR update (#6040)
d82b366 
* ci(lwd): run the send transactions test on each PR update

The send transactions test was moved to the main branch in #5480 because
it was very slow.

It's much faster (~30m) with #5015 and now it can be run for every PR
update again.

* fix(actions): remove references to the workflow_dispatch
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

@gustavovalverde gustavovalverde Awaiting requested review from gustavovalverde

Assignees
No one assigned
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement C-testing Category: These are tests C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-cost Zebra infrastructure costs I-slow Problems with performance or responsiveness
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move the send transactions test to the main branch
2 participants
@arya2 @teor2345