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

Revert "ref(workflow): move most scripts to their own executables" #8132

Closed
wants to merge 1 commit into from

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jan 2, 2024

Motivation

CI has been failing with an internal GitHub error since PR #8005 merged. Since people are away, the simplest way to fix this is to revert it for now.

Reverts #8005
Close #8123

@teor2345 teor2345 added C-bug Category: This is a bug A-devops Area: Pipelines, CI/CD and Dockerfiles P-High 🔥 I-integration-fail Continuous integration fails, including build and test failures labels Jan 2, 2024
@teor2345 teor2345 self-assigned this Jan 2, 2024
@teor2345 teor2345 requested a review from a team as a code owner January 2, 2024 07:02
@teor2345 teor2345 requested review from oxarbitrage and removed request for a team January 2, 2024 07:02
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 2, 2024
@teor2345
Copy link
Contributor Author

teor2345 commented Jan 2, 2024

If this CI passes without errors, we can merge this PR. We need to manually scroll down and check it for errors, because they don't reliably show up in the CI summary or auto-opened tickets:
https://github.com/ZcashFoundation/zebra/actions/runs/7383079119?pr=8132

@teor2345
Copy link
Contributor Author

teor2345 commented Jan 2, 2024

That worked, and I manually checked all of the other CI runs for this PR, they also passed.

Copy link
Member

@gustavovalverde gustavovalverde left a comment

Choose a reason for hiding this comment

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

Fixing it this way would revert improvements made to workflows that are not failing. From the 15+ impacted workflow files here, 1 workflow is failing. I'd prefer to tackle this specific issue and not revert the whole PR. I'll prioritize this

@teor2345
Copy link
Contributor Author

teor2345 commented Jan 4, 2024

Fixing it this way would revert improvements made to workflows that are not failing. From the 15+ impacted workflow files here, 1 workflow is failing. I'd prefer to tackle this specific issue and not revert the whole PR. I'll prioritize this

My goal was restoring test coverage as reliably and quickly as possible. This PR does that.

If we merge this PR, then the rest of the team doesn't need to worry about this bug, and you can take whatever time you need to fix the bugs in that workflow and its sub-workflows.

So I'd prefer a quick solution that makes all workflows functional. We can always add back the improvements when they are working correctly.

Would you be ok with reverting the changes to ci-integration-tests-gcp.yml and the workflows it depends on instead?

@gustavovalverde
Copy link
Member

My goal was restoring test coverage as reliably and quickly as possible. This PR does that.

The odd thing here is that those failures are not being reflected in PR or main checks
image

As far as I've been able to identify the workflow is getting a failure, but all jobs are ending successfully (so the final check result is a successful run) even on the commit that opened the issue: d1e72c1

Would you be ok with reverting the changes to ci-integration-tests-gcp.yml and the workflows it depends on instead?

Yes, I'd be ok reverting the changes in sub-deploy-integration-tests-gcp.yml (which is the sub-workflow being executed), but I'd prefer to try a fix with tomorrow as a deadline, and if I can't fix it, we merge the partial revert.

@teor2345
Copy link
Contributor Author

teor2345 commented Jan 4, 2024

Ok, happy to leave the fix or the partial revert to you.

@teor2345
Copy link
Contributor Author

teor2345 commented Jan 5, 2024

We don't need reminders for this, we're still deciding how to fix it.

@mergify mergify bot closed this in #8133 Jan 7, 2024
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-bug Category: This is a bug C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-integration-fail Continuous integration fails, including build and test failures no-review-reminders Turn off review reminders
Projects
None yet
2 participants