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

Allowing constructing CredentialsFile from string #201

Merged
merged 1 commit into from
Oct 13, 2023
Merged

Allowing constructing CredentialsFile from string #201

merged 1 commit into from
Oct 13, 2023
+161 −6

Conversation

benjaminch
Copy link
Contributor

@benjaminch benjaminch commented Oct 11, 2023 •
edited
Loading

PR aiming to tackle issue described here: #200

@yoshidan yoshidan added the safe to test safe to test label Oct 12, 2023
@yoshidan
Copy link
Owner

Please run

cargo clippy --fix
cargo fmt

to fix CI.

@benjaminch
Copy link
Contributor Author

Oupsie ! Fixing it now :)

@benjaminch
Copy link
Contributor Author

done !

@yoshidan yoshidan added safe to test safe to test and removed safe to test safe to test labels Oct 13, 2023
@yoshidan
Copy link
Owner

Overriding environment variables cause other tests to fail.
In the scope of your addition, only test_credentials_file_new_from_str is sufficient as a test, so could you please remove the other tests, leaving only test_credentials_file_new_from_str for the time being? I would appreciate it if you could delete the other tests.

@benjaminch
Copy link
Contributor Author

Hey @yoshidan !
Would it be ok to use a dedicated env var crates tackling those challenges?
It will allow to set env vars, test it without any issue having tests running in parallel?

It allows to keep tests on this part :)

Cheers

@yoshidan yoshidan added safe to test safe to test and removed safe to test safe to test labels Oct 13, 2023
yoshidan
yoshidan reviewed Oct 13, 2023
View reviewed changes
foundation/auth/Cargo.toml Outdated
@@ -36,7 +36,9 @@ hex = { version = "0.4", optional = true }
tokio = { version = "1.32", features = ["test-util", "rt-multi-thread", "macros"]}
tracing-subscriber = {version="0.3", features=["env-filter","std"]}
ctor = "0.1"
serial_test = "0.9"
tempdir = "0.3.7"
Copy link
Owner

Choose a reason for hiding this comment

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

Cloud you remove tempdir?
tempdir is archived and has volunerability.
https://rustsec.org/advisories/RUSTSEC-2018-0017.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, good catch, thanks :)

@yoshidan yoshidan added safe to test safe to test and removed safe to test safe to test labels Oct 13, 2023
@yoshidan yoshidan merged commit ff45202 into yoshidan:main Oct 13, 2023
@yoshidan
Copy link
Owner

Thanks!

@benjaminch benjaminch mentioned this pull request Oct 13, 2023
Closed
@benjaminch
Copy link
Contributor Author

Hey @yoshidan ! Would you mind creating a new release with this change?
Thanks a lot :)

@yoshidan
Copy link
Owner

Hi @benjaminch
I published new release now.

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

@yoshidan yoshidan yoshidan left review comments

Assignees
No one assigned
Labels
safe to test safe to test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@benjaminch @yoshidan