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

Rustls upgrade fix #1403

Closed
wants to merge 5 commits into from
Closed

Rustls upgrade fix #1403

wants to merge 5 commits into from

Conversation

nachog00
Copy link
Contributor

@nachog00 nachog00 commented Sep 18, 2024 •
edited
Loading

This is the simplest solution that gets rid of the error: #1389

thread 'main' panicked at /home/chona/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustls-0.23.12/src/crypto/mod.rs:259:14:
no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point

on cargo run.

I'm not sure if ring is the option we want nor if run_cli is the right place to call that install.

@nachog00 nachog00 self-assigned this Sep 18, 2024
@zancas zancas marked this pull request as ready for review September 18, 2024 15:30
@zancas
Copy link
Member

zancas commented Sep 18, 2024

Have you run the full test suite against this feature locally?

If yes, did all the tests pass?

@zancas
Copy link
Member

zancas commented Sep 18, 2024

This is blocked on #1401

zancas
zancas previously approved these changes Sep 18, 2024
View reviewed changes
zingocli/src/lib.rs
@@ -513,6 +514,11 @@ fn dispatch_command_or_start_interactive(cli_config: &ConfigTemplate) {

/// TODO: Add Doc Comment Here!
Copy link
Member

Choose a reason for hiding this comment

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

Upgrade the doc-comment, to something better.

Please add a doc-test.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is the entrypoint to the CLI so i'm not sure we can add a doc-test for this fn

zingocli/src/lib.rs Outdated Show resolved Hide resolved
@fluidvanadium
Copy link
Contributor

This is blocked on #1401

not exactly. however
allowing Nacho to run the full test suite is maximum priority and will have to happen before we can expect him to perfect this fix

@fluidvanadium fluidvanadium mentioned this pull request Sep 18, 2024
Closed
@fluidvanadium fluidvanadium mentioned this pull request Sep 18, 2024
Closed
Oscar-Pepper
Oscar-Pepper requested changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@Oscar-Pepper Oscar-Pepper left a comment

Choose a reason for hiding this comment

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

Sorry I was not aware of the state of this PR when I wrote #1426. This looks good but we will also have to delete the crypto provider install from netutils as this is called multiple times which causes an error. Would you mind if we closed this for #1426 ?

zingocli/src/lib.rs
@@ -513,6 +514,11 @@ fn dispatch_command_or_start_interactive(cli_config: &ConfigTemplate) {

/// TODO: Add Doc Comment Here!
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the entrypoint to the CLI so i'm not sure we can add a doc-test for this fn

zingocli/src/lib.rs
@@ -11,6 +11,7 @@ use std::sync::Arc;
use log::{error, info};

use clap::{self, Arg};
use rustls;
Copy link
Contributor

Choose a reason for hiding this comment

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

this line should not be necessary as you are calling rustls::crypto::ring::default_provider().install_default() with its full path

@nachog00
Copy link
Contributor Author

Sorry I was not aware of the state of this PR when I wrote #1426. This looks good but we will also have to delete the crypto provider install from netutils as this is called multiple times which causes an error. Would you mind if we closed this for #1426 ?

I'm fine with closing this one. Didn't do it already yesterday only cause @zancas asked me to add a test, documenting and some other stuff on #1426 and I wasnt sure how to collaborate on your PR. What is our flow in those cases? commit directly on it? PR on it? So far i got working on a local branch and was planning on PRing into yours.

@Oscar-Pepper
Copy link
Contributor

Sorry I was not aware of the state of this PR when I wrote #1426. This looks good but we will also have to delete the crypto provider install from netutils as this is called multiple times which causes an error. Would you mind if we closed this for #1426 ?

I'm fine with closing this one. Didn't do it already yesterday only cause @zancas asked me to add a test, documenting and some other stuff on #1426 and I wasnt sure how to collaborate on your PR. What is our flow in those cases? commit directly on it? PR on it? So far i got working on a local branch and was planning on PRing into yours.

I commented on your PR as I think a doc test is difficult in this case but good practice for functions that can be unit tested. So in most cases the flow would be to PR into my branch. However, in this case I am happy for you to just PR into dev and I'll close mine or push directly to my branch if your have collaborator rights (I'm not sure if you being part of the organisation allows this by default). It's up to you really

@Oscar-Pepper
Copy link
Contributor

closed for #1426

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Oscar-Pepper Oscar-Pepper Oscar-Pepper requested changes

@zancas zancas zancas left review comments

Assignees

@sebastianlujan sebastianlujan

@nachog00 nachog00

@fluidvanadium fluidvanadium

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nachog00 @zancas @fluidvanadium @Oscar-Pepper @sebastianlujan