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

cli: allow saving collector credential to file; update help text #617

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

inahga
Copy link
Contributor

@inahga inahga commented Nov 7, 2023

Alternative approach to divviup/janus#2221, let the user provide --save to save the correctly encoded credentials to file, and make it more explicit as to what portions of the CLI output need to be saved.

Also update the help text to use the collector credential nomenclature.

@inahga inahga marked this pull request as ready for review November 7, 2023 16:16
@inahga inahga requested review from jbr and a team as code owners November 7, 2023 16:16
@inahga
Copy link
Contributor Author

inahga commented Nov 21, 2023

@jbr any thoughts on this PR?

// of the output settings of this CLI. The credential file should be treated as
// opaque, so we don't grant user control over its encoding.
if save {
let mut file = File::create(name + ".json")?;
Copy link
Contributor

@jbr jbr Nov 21, 2023

Choose a reason for hiding this comment

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

This seems like an unconventional way to construct a path in rust (using +). I'm also slightly concerned that we haven't guarded against name including dangerous path characters like slashes, dots, and tildes. On one hand, people are unlikely to script this particular part of divviup-api with untrusted inputs. On the other, we probably should do something reasonable regardless of the --name.

Suggested change
let mut file = File::create(name + ".json")?;
let path = std::path::Path::new(name).with_extension("json");
let mut file = File::create(path)?;

or maybe

Suggested change
let mut file = File::create(name + ".json")?;
let path = std::env::current_dir()?.with_file_name(name).with_extension("json");
let mut file = File::create(path)?;

or with the same behavior but slightly more conventional appearance:

Suggested change
let mut file = File::create(name + ".json")?;
let mut file = File::create(format!("{name}.json"))?;

Copy link
Contributor

@jbr jbr Nov 21, 2023

Choose a reason for hiding this comment

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

we may also want something like

println!("Saved collector credential to {path}. Keep this somewhere safe!");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I liked the current_dir()... approach, since that offloads all the platform-specific gotchas onto std.

@inahga inahga requested a review from jbr November 22, 2023 19:20
Copy link
Contributor

@jbr jbr left a comment

Choose a reason for hiding this comment

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

🚀

@inahga inahga merged commit fbfa12d into main Nov 27, 2023
6 checks passed
@inahga inahga deleted the inahga/collector-credentials branch November 27, 2023 15:42
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.

2 participants