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

Load token via QLTY_TOKEN if provided #1410

Merged
merged 2 commits into from
Jan 7, 2025
Merged

Load token via QLTY_TOKEN if provided #1410

merged 2 commits into from
Jan 7, 2025

Conversation

lsegal
Copy link
Member

@lsegal lsegal commented Jan 7, 2025

No description provided.

@lsegal lsegal self-assigned this Jan 7, 2025
@lsegal lsegal requested a review from a team January 7, 2025 04:19
brynary
brynary approved these changes Jan 7, 2025
View reviewed changes
Copy link
Member

@brynary brynary left a comment

Choose a reason for hiding this comment

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

Added one question about env var semantics, but LGTM! In short, is provided, the env var always wins

qlty-cloud/src/auth/mod.rs Outdated
pub fn load_or_retrieve_auth_token() -> Result<String> {
if let Ok(token) = env::var(TOKEN_ENV_VAR) {
// bypass validation when env var is set since this is an intentional override of credential lookup
return Ok(token);
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit of a dumb question, but is it possible to have an env var be set but empty? Should we add an is_empty() check here?

lsegal
lsegal commented Jan 7, 2025
View reviewed changes
Copy link
Member Author

@lsegal lsegal left a comment

Choose a reason for hiding this comment

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

This is a bit of a dumb question, but is it possible to have an env var be set but empty? Should we add an is_empty() check here?

Yes this is a good callout, I had this in another version and forgot to port it.

@qltysh qltysh
Copy link
Contributor

qltysh bot commented Jan 7, 2025

Not applicable. There was no coverage data reported for the files in this diff.

@lsegal lsegal merged commit e8feb1b into ls/auth-cli-token-set Jan 7, 2025
8 of 9 checks passed
@lsegal lsegal deleted the ls/qlty-token-env branch January 7, 2025 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brynary brynary brynary approved these changes

Assignees

@lsegal lsegal

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lsegal @brynary