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

remove consensus, extraneous parameter for update_view, repoint deps #3755

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

pls148
Copy link
Contributor

@pls148 pls148 commented Oct 11, 2024

Closes #3640

This PR:

This PR removes the consensus module (deprecated in favor of consensus2) and removes an extra parameter for update_view. Also removes references to the "dependency-tasks" feature.

This PR does not:

This does NOT rename the consensus2 module to consensus, nor change any other types. That will come in a later PR.

Key places to review:

@CLAassistant
Copy link

CLAassistant commented Oct 11, 2024

CLA assistant check
All committers have signed the CLA.

@pls148 pls148 force-pushed the ps/3640 branch 6 times, most recently from f81c9ca to 300d0eb Compare October 11, 2024 00:10
@bfish713
Copy link
Collaborator

This looks good. However I think now the CI is redundant. The ci jobs for test and test-dependency-tasks are running the same tests now I think. In a follow up can you confirm that the sets of tests are the same and remove one of them?

Future follow up is to pick out the now unused or misused types/functions and types in task-impl helpers (and other places)

Copy link
Contributor

@ss-es ss-es left a comment

Choose a reason for hiding this comment

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

I think the feature also appears in a few Cargo.toml files, and should be removed from there

For CI, you can also just delete the block called test-dependency-tasks in .github/workflows/build-and-test.yml (keeping the regular tests, which now always run the dependency tasks)

@pls148
Copy link
Contributor Author

pls148 commented Oct 11, 2024

I've added another commit which removes dependency-tasks from CI and Cargofiles, for the rest of the stuff I'm going to make a follow-up PR to avoid blocking 3738

Copy link
Contributor

@ss-es ss-es left a comment

Choose a reason for hiding this comment

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

looks good to me!

I didn't look at the original PR in depth, but I think Brendon did so should be good to merge

crates/testing/tests/tests_1/upgrade_task_with_proposal.rs Outdated Show resolved Hide resolved
crates/testing/tests/tests_1/upgrade_task_with_vote.rs Outdated Show resolved Hide resolved
@pls148 pls148 merged commit a4604dd into main Oct 11, 2024
24 checks passed
@pls148 pls148 deleted the ps/3640 branch October 11, 2024 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CX_HARDENING] - Delete Legacy Consensus
4 participants