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): Remove duplicate CI jobs for getblocktemplate-rpcs #7753

Merged
merged 17 commits into from
Oct 22, 2023
Merged

change(ci): Remove duplicate CI jobs for getblocktemplate-rpcs #7753

merged 17 commits into from
Oct 22, 2023

Conversation

teor2345
Copy link
Collaborator

@teor2345 teor2345 commented Oct 17, 2023 •
edited
Loading

Motivation

Now that the getblocktemplate-rpcs feature is on by default, we want to remove redundant tests.

Close #7362.

Depends-On: #7740
Depends-On: #7616

Complex Code or Requirements

It's tricky to keep track of all the different locations we use Rust features, and keep them all consistent. So I turned them into repository variables.

Solution

Preparation:

  • turn Rust features into repository variables
  • make features consistent across different workflows
  • remove redundant features in entrypoint.sh

Changes:

  • remove duplicate CI docker job
  • remove duplicate OS job
  • rename experimental release build job

Admin Tasks

  • Remove branch protection rules for jobs that don't exist any more

Author Tasks

  • This PR needs to be manually rebased after the dependent PRs merge

Review

This is a routine cleanup.

Reviewer Checklist

  • 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

Some of these jobs could move into entrypoint.sh, I added TODOs.

@teor2345 teor2345 added A-devops Area: Pipelines, CI/CD and Dockerfiles C-cleanup Category: This is a cleanup P-Low ❄️ C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Oct 17, 2023
@teor2345 teor2345 self-assigned this Oct 17, 2023
@github-actions github-actions bot added the extra-reviews This PR needs at least 2 reviews to merge label Oct 17, 2023
@teor2345 teor2345 marked this pull request as ready for review October 17, 2023 00:19
@teor2345 teor2345 requested a review from a team as a code owner October 17, 2023 00:19
@teor2345 teor2345 requested review from upbqdn and removed request for a team October 17, 2023 00:19
@mpguerra mpguerra linked an issue Oct 17, 2023 that may be closed by this pull request
Remove extra tests for getblocktemplate-rpcs feature #7362
Closed
@teor2345
Copy link
Collaborator Author

The failures here are probably from the base branch.

@teor2345 teor2345 requested review from a team as code owners October 18, 2023 04:24
@teor2345 teor2345 requested review from arya2 and removed request for a team October 18, 2023 04:24
@teor2345 teor2345 changed the base branch from remove-gbt-ci-base to main October 18, 2023 08:11
upbqdn
upbqdn reviewed Oct 18, 2023
View reviewed changes
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

GitHub says that the env context in not allowed in the release-binaries.yml workflow. I don't see why, though.

@teor2345
Copy link
Collaborator Author

GitHub says that the env context in not allowed in the release-binaries.yml workflow. I don't see why, though.

Can you show me this error? I can't find it in the list of jobs in the PR, or in the list of repository actions.

@upbqdn
Copy link
Member

upbqdn commented Oct 18, 2023

Sure, it's at line 48 in release-binaries.yml when you open the diff: https://github.com/ZcashFoundation/zebra/pull/7753/files#diff-07d8c704ff21e83c1d4e79f9f7bc1472dc35deac2cbadc8b53b59131e30879c1.

@teor2345
Copy link
Collaborator Author

GitHub says that the env context in not allowed in the release-binaries.yml workflow. I don't see why, though.

Apparently it's not allowed in with::

You can use the env context in any key in a workflow step except for the id and uses keys.
https://docs.github.com/en/actions/learn-github-actions/contexts#env-context

That's ok, I just used the vars directly.

upbqdn
upbqdn previously approved these changes Oct 18, 2023
View reviewed changes
@gustavovalverde
Copy link
Member

Test all and Test with fake activation heights are consistently failing here, so I'll guess this is not a CI issue, but an issue with this PR

@teor2345
Copy link
Collaborator Author

docker: Error response from daemon: Conflict. The container name "/zebrad-tests" is already in use by container "aa081fba8c1295879191dd359ce35468a9b50498643916981d9f52dbb33683b8". You have to remove (or rename) that container to be able to reuse that name.

Yep, I just need a different container name.

@gustavovalverde Mergify
Copy link
Member

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Oct 22, 2023

refresh

✅ Pull request refreshed

@gustavovalverde Mergify
Copy link
Member

@Mergifyio queue batched

@mergify
Copy link
Contributor

mergify bot commented Oct 22, 2023

queue batched

❌ Branch protection setting 'linear history' conflicts with Mergify configuration

Branch protection setting 'linear history' works only if merge_method: squash or merge_method: rebase.

@gustavovalverde Mergify
Copy link
Member

@Mergifyio requeue batched

@mergify
Copy link
Contributor

mergify bot commented Oct 22, 2023

requeue batched

❌ This pull request head commit has not been previously disembarked from queue.

mergify bot added a commit that referenced this pull request Oct 22, 2023
@mergify
Merge of #7753
90fc3fa
@mergify mergify bot mentioned this pull request Oct 22, 2023
Closed
@mergify mergify bot merged commit 60b447b into main Oct 22, 2023
136 checks passed
@mergify mergify bot deleted the remove-gbt-ci branch October 22, 2023 15:40
@teor2345 teor2345 mentioned this pull request Oct 25, 2023
Merged
@teor2345 teor2345 mentioned this pull request Nov 5, 2023
Merged
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@upbqdn upbqdn upbqdn approved these changes

@gustavovalverde gustavovalverde gustavovalverde approved these changes

@arya2 arya2 Awaiting requested review from arya2 arya2 was automatically assigned from ZcashFoundation/unsafe-rust-reviewers

Assignees

@teor2345 teor2345

Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-cleanup Category: This is a cleanup C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG extra-reviews This PR needs at least 2 reviews to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove extra tests for getblocktemplate-rpcs feature
3 participants
@teor2345 @upbqdn @gustavovalverde