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

Deprecate certificate pending #1972

Merged
merged 6 commits into from
Oct 3, 2024
Merged

Deprecate certificate pending #1972

merged 6 commits into from
Oct 3, 2024

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Oct 1, 2024 •
edited
Loading

Content

This PR deprecate the CertificatePendingMessage both in rust code and in the openapi. It also remove the remaining certificate pending code in mithril-signer.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

Comments

This is a follow up on #1970, so it must be merged after it.

Issue(s)

Closes #1925

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 1, 2024 •
edited
Loading

Test Results

    4 files  ±0     54 suites  ±0   9m 27s ⏱️ -24s
1 329 tests  - 9  1 329 ✅  - 9  0 💤 ±0  0 ❌ ±0 
1 540 runs   - 9  1 540 ✅  - 9  0 💤 ±0  0 ❌ ±0 

Results for commit 5cef73c. ± Comparison against base commit 2cc3fd3.

♻️ This comment has been updated with latest results.

@Alenar Alenar force-pushed the djo/1925/deprecate_certificate_pending branch from 60fe7bd to c4a3558 Compare October 1, 2024 16:32
@Alenar Alenar temporarily deployed to testing-sanchonet October 1, 2024 16:40 — with GitHub Actions Inactive
jpraynaud
jpraynaud approved these changes Oct 1, 2024
View reviewed changes
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM

mithril-common/src/messages/certificate_pending.rs Outdated Show resolved Hide resolved
sfauvel
sfauvel approved these changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

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

LGTM

mithril-common/src/test_utils/apispec.rs
@@ -433,12 +443,12 @@ components:
fn test_validate_ok_when_request_without_body_and_expects_response() {
APISpec::from_file(&APISpec::get_default_spec_file())
.method(Method::GET.as_str())
.path("/certificate-pending")
.path("/")
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be interesting to not use the openapi.yaml of the project for those tests.
We are just testing our tools here.
This PR would not have to modify this file if we have used a separate file or a file created by the test.
Ideally (probably too much work), we could create a refactoring PR to not use the production file in those tests and then this PR changes only what is impacted by the modification.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree with that 👍 , as you say we should do a refactoring PR to solve that problem.

mithril-signer/src/services/aggregator_client.rs
@@ -214,36 +205,6 @@ impl AggregatorClient for AggregatorHTTPClient {
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

The /certificate_pending still exist in openapi.yaml even if it was deprecated.
Is it normal to remove all tests ? What happen when the route is called ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it's normal since one of this PR goal is to remove support for the certificate pending in the mithril-signer. Its the first step that will allow to remove it from the aggregator and the openapi in the near future.

@Alenar Alenar force-pushed the ensemble/1925/signer_self_compute_signed_entity branch from 6353393 to 2cc3fd3 Compare October 2, 2024 15:26
@Alenar
refactor: remove all pending_certificate code from mithril-signer
c506bee 
As the signer can now compute what to sign on its own.
@Alenar
tests: remove reference to certificate pending in apispecs tests
411f066 
As they will be removed in the future
@Alenar Alenar force-pushed the djo/1925/deprecate_certificate_pending branch from c4a3558 to 6db1cd6 Compare October 2, 2024 16:15
@Alenar Alenar force-pushed the djo/1925/deprecate_certificate_pending branch from 6db1cd6 to 92b0a16 Compare October 2, 2024 16:27
@Alenar Alenar temporarily deployed to testing-sanchonet October 2, 2024 17:00 — with GitHub Actions Inactive
@Alenar
chore: bump crates and openapi versions
5b70057 
* OpenApi from `0.1.31` to `0.1.32`
* Mithril-aggregator from `0.5.72` to `0.5.73`
* Mithril-signer from `0.2.190` to `0.2.191`
* Mithril-common from `0.4.62` to `0.4.63`
* Mithril-end-to-end from `0.4.32` to `0.4.33`
@Alenar
chore: update changelog
5cef73c
@Alenar Alenar force-pushed the djo/1925/deprecate_certificate_pending branch from 92b0a16 to 5cef73c Compare October 3, 2024 08:55
Base automatically changed from ensemble/1925/signer_self_compute_signed_entity to main October 3, 2024 09:06
@Alenar Alenar merged commit a80a413 into main Oct 3, 2024
38 checks passed
@Alenar Alenar deleted the djo/1925/deprecate_certificate_pending branch October 3, 2024 09:07
@Alenar Alenar temporarily deployed to testing-sanchonet October 3, 2024 09:08 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfauvel sfauvel sfauvel approved these changes

@jpraynaud jpraynaud jpraynaud approved these changes

@dlachaume dlachaume dlachaume approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Signer computes what to sign on its own
4 participants
@Alenar @sfauvel @jpraynaud @dlachaume