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

ref(docker): combine test and prod entrypoints into one #7660

Merged
merged 22 commits into from
Oct 11, 2023
Merged

ref(docker): combine test and prod entrypoints into one #7660

merged 22 commits into from
Oct 11, 2023

Conversation

gustavovalverde
Copy link
Member

Motivation

Maintaining both entrypoint is not necessary, and having a single one help with refactors like:

Fixes: #7639

Complex Code or Requirements

  • A whole file disappears and it's merged into another
  • Some extra changes were added to make the whole file less verbose and fix linting issues

Solution

  • Uses -x instead of echoing the variables values
  • Sets default values where required
  • Create a function to list directories
  • Create a function to run cargo tests
  • Use a better approach to handle different options in the case manegement for tests and production
  • Replaces all instances of runtime-entrypoint.sh with entrypoint.sh

Review

  • Verify all tests are passing in this PR
  • A long lived instance must be deployed from this PR to validate CD configuration

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

Use arguments instead of variables, to run tests, based on the following comment:

@gustavovalverde gustavovalverde added A-devops Area: Pipelines, CI/CD and Dockerfiles P-Critical 🚑 I-usability Zebra is hard to understand or use C-feature Category: New features labels Oct 2, 2023
@gustavovalverde gustavovalverde self-assigned this Oct 2, 2023
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 2, 2023
@teor2345
Copy link
Collaborator

teor2345 commented Oct 3, 2023

It might help to run a full sync with these script changes, to check that it works. But that's also a very slow test. What do you think?

I don't mind either way, as long as the full sync is working on main, we can wait for the end of the week. (But if it's not working on main that's a blocker.)

teor2345
teor2345 reviewed Oct 3, 2023
View reviewed changes
docker/entrypoint.sh Outdated Show resolved Hide resolved
@gustavovalverde
Copy link
Member Author

It might help to run a full sync with these script changes, to check that it works. But that's also a very slow test. What do you think?

If other tests are passing, it should be the same behavior with the full sync. If my any means this causes an odd behavior (which I wouldn't expect) it should be quick to fix or revert.

@teor2345
Copy link
Collaborator

teor2345 commented Oct 3, 2023

It might help to run a full sync with these script changes, to check that it works. But that's also a very slow test. What do you think?

If other tests are passing, it should be the same behavior with the full sync. If my any means this causes an odd behavior (which I wouldn't expect) it should be quick to fix or revert.

Ok, but the full sync hasn't passed since 1 September, before the self-hosted runners changes. So did you want to wait for a successful full sync on main before merging these changes?

I am concerned we might end up with a stack of changes which might all contribute to the ongoing failures, and it's difficult to work out which ones are causing it. (Like we have in ticket #7618 in the Zebra Rust code right now!)

teor2345
teor2345 reviewed Oct 3, 2023
View reviewed changes
docker/entrypoint.sh Outdated Show resolved Hide resolved
@gustavovalverde
Copy link
Member Author

So did you want to wait for a successful full sync on main before merging these changes?

Oh yeah, that's ok, this doesn't has to merged beforehand as we'll all be focusing on fixing the full sync test.

In another note, the long-live instance is working.

Here's the workflow run: https://github.com/ZcashFoundation/zebra/actions/runs/6386506331

And the instance logs.
image

This was referenced Oct 3, 2023
Closed
Closed
@gustavovalverde gustavovalverde marked this pull request as ready for review October 5, 2023 12:59
@gustavovalverde gustavovalverde requested review from a team as code owners October 5, 2023 12:59
@gustavovalverde gustavovalverde requested review from arya2 and removed request for a team October 5, 2023 12:59
@gustavovalverde gustavovalverde mentioned this pull request Oct 5, 2023
Merged
6 tasks
@teor2345 teor2345 removed the request for review from a team October 9, 2023 03:25
@teor2345 teor2345 added the do-not-merge Tells Mergify not to merge this PR label Oct 9, 2023
@gustavovalverde gustavovalverde force-pushed the ref-docker-entrypoint branch 2 times, most recently from 14a9a0b to 6c7130f Compare October 9, 2023 18:41
@teor2345 teor2345 removed the do-not-merge Tells Mergify not to merge this PR label Oct 9, 2023
@gustavovalverde
ref(docker): combine test and prod entrypoints into one
1f47193 
This makes the following changes
- Uses `-x` instead of echoing the variables values
- Sets default values where required
- Create a function to list directories
- Create a function to run cargo tests
- Use a better approach to handle different options in the `case` manegement for tests and production
- Replaces all instances of `runtime-entrypoint.sh` with `entrypoint.sh`
@gustavovalverde
chore: use 2 spaces insted of 4 for indentation
f3c85b4
@gustavovalverde
fix: add missing test-threads to fully_synced_rpc_
1561aaf
@gustavovalverde
fix: allow 3 arguments instead of 2 to handle fully_synced_rpc_ cor…
3c96797 
…rectly
@gustavovalverde
chore: remove extra file
b2fd228
@gustavovalverde
chore: add comments to main parts of the file
11d6881
@gustavovalverde
fix(ci): just create the $ZEBRA_CONF_PATH if $ENTRYPOINT_FEATURES…
172c576 
… is not set
@gustavovalverde
fix(entrypoint): just print ${ZEBRA_CONF_PATH} if exists
de9f8ea
@gustavovalverde
fix: missing condition
b8cafde
@gustavovalverde
ref: handle tests better if $ENTRYPOINT_FEATURES is set
60446c0
@gustavovalverde
fix(ci): We just want ZEBRA_CONF_PATH to be set in the release image
76e1f90
@gustavovalverde
fix(entrypoint): fix the overall case logic
a13831c
arya2
arya2 previously approved these changes Oct 11, 2023
View reviewed changes
@arya2 arya2 dismissed their stale review October 11, 2023 19:00

"A long lived instance must be deployed from this PR to validate CD configuration", but otherwise this PR looks great.

@mergify mergify bot merged commit fbd6b0f into main Oct 11, 2023
146 checks passed
@mergify mergify bot deleted the ref-docker-entrypoint branch October 11, 2023 19:04
@upbqdn upbqdn mentioned this pull request Oct 13, 2023
Merged
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 left review comments

@arya2 arya2 arya2 approved these changes

Assignees

@gustavovalverde gustavovalverde

Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-feature Category: New features 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.

devops: Use the same entrypoint.sh for all Docker stages
3 participants
@gustavovalverde @teor2345 @arya2