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: Wait for lightwalletd image rebuild if it has changed #4612

Closed
Tracked by #3096
teor2345 opened this issue Jun 14, 2022 · 3 comments
Closed
Tracked by #3096

ci: Wait for lightwalletd image rebuild if it has changed #4612

teor2345 opened this issue Jun 14, 2022 · 3 comments
Assignees
@gustavovalverde
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-bug Category: This is a bug I-integration-fail Continuous integration fails, including build and test failures lightwalletd any work associated with lightwalletd

Comments

@teor2345
Copy link
Contributor

teor2345 commented Jun 14, 2022 •
edited
Loading

Motivation

If we change the lightwalletd Docker image in a PR, we want to see those changes in the PR's tests. Otherwise we won't know if our fixes work.

If we don't change the image, the latest main branch image is fine.

Design

Before starting this PR, we want to add the job to the branch protection rules, so we don't merge broken code:

We might want to use the same dependency algorithm we use for the state cache:

  • wait for the image from the current PR and commit, if it is being rebuilt, then use it, or
  • use the image from main

Original Conversation

I'm not sure if this completely solves the problem. What happens if we need to update the image? The PR that changes it will use the latest image which will not be the updated one and it will fail CI.

I had the same thought, it makes testing a bit tricky.

Originally posted by @teor2345 in #4599 (comment)
@teor2345 teor2345 added C-bug Category: This is a bug A-devops Area: Pipelines, CI/CD and Dockerfiles P-Medium ⚡ lightwalletd any work associated with lightwalletd labels Jun 14, 2022
@ftm1000 ftm1000 added the S-needs-triage Status: A bug report needs triage label Jun 15, 2022
@gustavovalverde gustavovalverde self-assigned this Jun 20, 2022
@ftm1000 ftm1000 removed the S-needs-triage Status: A bug report needs triage label Jun 23, 2022
@teor2345 teor2345 added P-Medium ⚡ I-integration-fail Continuous integration fails, including build and test failures and removed P-Low ❄️ labels Aug 4, 2022
@teor2345
Copy link
Contributor Author

teor2345 commented Aug 4, 2022 •
edited
Loading

This caused a build failure on the main branch:

So I've set it as a medium priority.
(It's already scheduled for a sprint.)

@teor2345 teor2345 mentioned this issue Aug 4, 2022
Merged
1 task
@teor2345 teor2345 mentioned this issue Aug 25, 2022
Closed
76 tasks
gustavovalverde added a commit that referenced this issue Aug 29, 2022
@gustavovalverde
ci(build): allow to use lightwalletd images from PRs
6cc68ac 
Previous behavior:
If the lightwalledt Dockerfile was changed in a PR, we couldn't test the
changes from the actual branch as the image being pulled was the one from
the `main` branch.

Expected behavior:

If we change the lightwalletd Docker image in a PR, those changes in the
PR should be reflected in tests. Otherwise we won't know if our fixes work.

If the image is not changed, the latest `main` branch image is fine.

Solution:
When building Zebra, allow to build using a lightwalletd image built from
the actual working branch/PR if lightwalletd code has likely changed

Fixes #4612
@gustavovalverde gustavovalverde mentioned this issue Aug 29, 2022
Closed
2 tasks
@mpguerra mpguerra added this to Zebra Sep 22, 2022
@mpguerra mpguerra moved this to 🆕 New in Zebra Sep 22, 2022
@mpguerra mpguerra moved this from 🆕 New to 📋 Sprint Backlog in Zebra Sep 22, 2022
@teor2345
Copy link
Contributor Author

This isn't actually causing us any problems at the moment

@mpguerra mpguerra added the NU-6 Network Upgrade: NU6 specific tasks label Sep 27, 2022
@gustavovalverde
Copy link
Member

Work was started in #4980 (comment) but a refactor is needed to make builds interdependent

Repository owner moved this from 📋 Sprint Backlog to ✅ Done in Zebra Sep 28, 2022
@mpguerra mpguerra removed the NU-6 Network Upgrade: NU6 specific tasks label Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gustavovalverde gustavovalverde

Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-bug Category: This is a bug I-integration-fail Continuous integration fails, including build and test failures lightwalletd any work associated with lightwalletd
Projects
Zebra
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ci(build): allow to use lightwalletd images from PRs ZcashFoundation/zebra
4 participants
@mpguerra @teor2345 @gustavovalverde @ftm1000