af/init command #30
af/init command #30
Conversation
|
clarification: i do not care about code formatting feedback, such as the compiler warnings for the structs or the |
There was a problem hiding this comment.
Structure comments:
- So far, I've tried to keep the bulk of the logic for comands in their own folders, and
run()orchestrates which thing to call based on the command line flags (i.e.build::run()calls build_image() and then push_images() if--pushwas given). I think the*_init()functions and the struct definitions should go in their own folder/file to follow this. Thoughts from the rest of the team?
| s3_secretaccesskey: String | ||
| } | ||
|
|
||
| pub fn run(_interactive: &bool, _blank: &bool) // TODO: is there a way to set options at mutually exclusive? |
There was a problem hiding this comment.
There is, conflicts_with in clap-derive (docs)
| let flag_regex; | ||
| let registry_domain; | ||
| let registry_build_user; | ||
| let registry_build_pass; | ||
| let registry_cluster_user; | ||
| let registry_cluster_pass; | ||
| let defaults_difficulty; | ||
| let defaults_resources_cpu; | ||
| let defaults_resources_memory; | ||
| let mut points_difficulty = Vec::new(); | ||
| let mut deploy_profiles = Vec::new(); |
There was a problem hiding this comment.
This should probably be an mut init_vars struct instead of a bunch of individual variables, since these are all getting set into one after the prompts.
(ditto for points and profile sections)
| defaults_difficulty = inquire::Select::new("Please choose the default difficulty class:", points_difficulty.clone()) | ||
| .prompt()?; | ||
|
|
||
| // TODO: how much format validation should these two do now vs offloading to validate() later? current inquire replacement calls are temporary and do the zero checking, just grabbing a String | ||
| // defaults_resources_cpu = inquire::CustomType::<u64>::new("Default CPUs per challenge:") | ||
| // // default parser calls std::u64::from_str | ||
| // .with_error_message("Please type a valid number.") | ||
| // .with_help_message("The maximum limit of CPU resources per instance of challenge deployment (\"pod\").") | ||
| // .prompt()?; | ||
| defaults_resources_cpu = inquire::Text::new("Default limit of CPUs per challenge") | ||
| .with_help_message("The maximum limit of CPU resources per instance of challenge deployment (\"pod\").") | ||
| .prompt()?; | ||
|
|
||
| // defaults_resources_memory = inquire::CustomType::<String>::new("") | ||
| // .with_parser(&|i| | ||
| // { | ||
| // let re = Regex::new(r"^[0-9]+$") // TODO | ||
| // }) | ||
| defaults_resources_memory = inquire::Text::new("Default limit of memory per challenge") | ||
| .with_help_message("The maximum limit of memory resources per instance of challenge deployment (\"pod\").") | ||
| .prompt()?; |
There was a problem hiding this comment.
I'm not sure if these need to be prompted for. Most challenges are going to be fine with a reasonable default resource limit, and I think we should figure out what that default is and always include it. I think users will very rarely need to change these here for all challenges, and that can be left to manual edits.
| .with_help_message("The maximum limit of memory resources per instance of challenge deployment (\"pod\").") | ||
| .prompt()?; | ||
|
|
||
| println!("You can define several challenge difficulty classes below."); |
There was a problem hiding this comment.
Wrong header message?
| // TODO -- does not compile because options does not yet implement serialize | ||
| // comment out if wanting to test the rest of the code |
There was a problem hiding this comment.
Adding #[derive(Serialize)] to the other structs fixes this
| @@ -69,4 +69,17 @@ pub enum Commands { | |||
| #[arg(short, long)] | |||
| registry: bool, | |||
| }, | |||
|
|
|||
| /// Copy an initial rcds.yaml to the current working directory. | |||
There was a problem hiding this comment.
Since this command can do interactive prompts aside from non-interactive copy/write-out, this description should a bit more generic, something like
| /// Copy an initial rcds.yaml to the current working directory. | |
| /// Set up a new rcds.yaml config in the current working directory |
| /// If interactive is enabled, then it will prompt for the various fields of the config file. If left disabled, then it will copy it out with fake data of the expected format. | ||
| /// | ||
| /// If blank is enabled, then it will copy out the file without any fields set. If left disabled, it will write the default non-interactive example config to file. |
There was a problem hiding this comment.
These should be on the arguments since they are describing what they do.
|
|
||
| struct init_vars | ||
| { | ||
| flag_regex: String, // TODO: make all of these `str`s if it compiles |
There was a problem hiding this comment.
Structs need to own the data they contain, so these do all have to be Strings.
| // TODO: would the cluster not use a token of some sort? | ||
| registry_cluster_pass = inquire::Password::new("Container registry password (CLUSTER'S):") | ||
| .with_help_message("The cluster's password to the remote container registry, which it will use to pull containers from.") | ||
| .prompt()?; |
There was a problem hiding this comment.
Kubernetes uses the same base64("username:password") credential format as Docker does, so we're matching that here.
| registry_build_user = inquire::Text::new ("Container registry user (YOURS):") | ||
| .with_help_message("Your username to the remote container registry, which you will use to push containers to.") | ||
| .prompt()?; | ||
|
|
||
| // TODO: do we actually want to be in charge of these credentials vs letting the container building utility take care of it? | ||
| registry_build_pass = inquire::Password::new("Container registry password (YOURS):") | ||
| .with_help_message("Your password to the remote container registry, which you will use to push containers to.") // TODO: could this support username:pat too? |
There was a problem hiding this comment.
To me, "your" is a bit confusing since the config calls this build credentials -- "local/build" might work better?
Opening a premature PR on this so that I can get midway feedback and get most of my "TODO" questions answered this Sunday (not all of the TODOs are for y'all but most of them are, feel free to comment on any you want to give insight on). Please add your answers to the TODOs via comments in a review on this PR since I can't be there this Sunday