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): consolidate workflows based on their purpose #7616

Merged
merged 41 commits into from
Oct 18, 2023

Conversation

gustavovalverde
Copy link
Member

@gustavovalverde gustavovalverde commented Sep 25, 2023

Motivation

Some workflows have different requirements like resources, time and dependencies, and even have jobs with different objectives (in some cases, not related to other workflow jobs).

Fixes: #6166
Fixes: #6167
Depends-On: #7660

Specifications

Complex Code or Requirements

To comply with the naming convention, some jobs had to be moved from one workflow to another, and we have a completely new workflow, but no new jobs, which consolidates integration tests (sync tests) which required to be deployed in GCP

Solution

  • Rename the workflows to make their naming more consistent by adding a naming convention
  • Move jobs to the workflow which is more consistent with their objective

Review

  • Confirm that all workflows are working as expected (running and not failing)
  • Validate if the naming convention makes sense

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

This also renames the workflows to make their naming more consistent and adding a naming convention

Fixes: #6166
Fixes: #6167
@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 Sep 25, 2023
@gustavovalverde gustavovalverde requested review from a team as code owners September 25, 2023 08:41
@gustavovalverde gustavovalverde requested review from oxarbitrage and removed request for a team September 25, 2023 08:41
@gustavovalverde gustavovalverde self-assigned this Sep 25, 2023
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Sep 25, 2023
@upbqdn upbqdn added the do-not-merge Tells Mergify not to merge this PR label Sep 25, 2023
@teor2345

This comment was marked as resolved.

@teor2345

This comment was marked as resolved.

@upbqdn upbqdn removed the do-not-merge Tells Mergify not to merge this PR label Sep 25, 2023
@teor2345 teor2345 removed the request for review from a team September 26, 2023 06:16
@gustavovalverde
Copy link
Member Author

gustavovalverde commented Oct 17, 2023

EDIT: Actually, let's keep the test coverage as is, I'll make the workflow segregation in another PR fix: revert test coverage on CD

The Docker release image checks need to run on PRs so that we don't merge PRs that break the release images.

we need to check the release images in every PR, just like we did before.

If we don't want to release a production image with issues, I think running this in the main branch should be a good starting point, as we'll be warned when this test fails.

A more "elegant" solution is also making the push on Docker Hub a dependency on this tests, at release time (but that's a refactor on its own). Otherwise, if we're going to test this in PRs I think this file should be segregated between a workflow that is meant to deploy instances (CD), and other that is meant to run tests on the test or release image (CI)

@gustavovalverde
Copy link
Member Author

All recommendations were applied

@gustavovalverde
Copy link
Member Author

gustavovalverde commented Oct 17, 2023

I made a last fix to the docker config test to accept different images. Even though this was (mainly) required in this spots: fd6aaf2#diff-d43d0df5f20e7f637ae74715b01cc8a0b644574a546a5242c78b9b8d4bab9571R217

I added it to other places for consistency

@gustavovalverde
Copy link
Member Author

If this test fails https://github.com/ZcashFoundation/zebra/actions/runs/6553940936/job/17800228563?pr=7616 after fix(dockerfile): use variables or default for config path and file it might because this file does not exists in the release image: zebrad/tests/common/configs/v1.0.0-rc.2.toml

If that's the case, we can remove this test from CD, or use a file generated by Zebra, different than the one created by the entrypoint

@teor2345
Copy link
Contributor

teor2345 commented Oct 18, 2023

I'm going to delete the old branch protection rules now, so this PR and other PRs can merge.

Admin: This PR needs 4 new branch protection rules after it merges:

  • Test CD default Docker config file / Test default-conf in Docker
  • Test CD custom Docker config file / Test custom-conf in Docker
  • Test CI default Docker config file / Test default-conf in Docker
  • Test CI custom Docker config file / Test custom-conf in Docker

I'll go update the patch jobs to match in this PR now.

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.

Thank you for all your work on this, I think we're good to go here

@teor2345
Copy link
Contributor

If we don't want to release a production image with issues, I think running this in the main branch should be a good starting point, as we'll be warned when this test fails.

A more "elegant" solution is also making the push on Docker Hub a dependency on this tests, at release time (but that's a refactor on its own).

We like to stop these kinds of bugs before we merge a PR, because fixing them after we merge or tag the release takes extra time. This is particularly important if we're trying to get a release out quickly to meet a deadline or fix a security issue.

It's ok to also test in the main branch if you want, but this seems less important (maybe optional?). It helps us work out if an issue is in multiple PRs, or the main branch and all PRs.

Otherwise, if we're going to test this in PRs I think this file should be segregated between a workflow that is meant to deploy instances (CD), and other that is meant to run tests on the test or release image (CI)

That seems like a great idea! There's no need to have the release depend on the release image tests, as long as we're sure the tested release image is always the same as the DockerHub push released image. (If we're testing every PR, it's unlikely that the test will fail during the release itself.)

mergify bot added a commit that referenced this pull request Oct 18, 2023
@mergify mergify bot merged commit fc0133e into main Oct 18, 2023
179 checks passed
@mergify mergify bot deleted the refactor-workflows branch October 18, 2023 06:16
@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 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.

actions: segregate workflows based on their purpose actions: use a name convention for workflow files/names
4 participants