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

Hab-sup-run arguments modified. #8464

Merged
merged 1 commit into from
Mar 31, 2022
Merged

Hab-sup-run arguments modified. #8464

merged 1 commit into from
Mar 31, 2022

Conversation

dikshagupta1
Copy link
Contributor

Raise usage errors if inappropriate hab svc load options are given in hab sup run.

Signed-off-by: dikshagupta1 diksha.gupta@progress.com

@netlify
Copy link

netlify bot commented Mar 16, 2022 •
edited
Loading

👷 Deploy Preview for chef-habitat processing.

Name Link
🔨 Latest commit cd05d44
🔍 Latest deploy log https://app.netlify.com/sites/chef-habitat/deploys/62455c0662db53000881b1d1

@mwrock
Copy link
Contributor

mwrock commented Mar 18, 2022

I did a little digging and it looks like the reason hab sup run with no args complains about the missing pkg_ident_or_artifact arg comes back to the default values. Adding requires = "pkg_ident_or_artifact" to all fields with no default values works as expected for me. I think the final solution will likely need to put the SharedLoad fields into SupRun like you have here. You could eliminate the default values from the structopt attributes in SupRun and then have your from implementation handle injecting the defaults if the fields are None.

As for handling the issue with the move compile errors, see my comments in the code for how to deal with that.

mwrock
mwrock reviewed Mar 18, 2022
View reviewed changes
components/hab/src/command/launcher.rs Outdated
@@ -22,7 +23,8 @@ const LAUNCH_PKG_IDENT: &str = "core/hab-launcher";

pub async fn start(ui: &mut UI, sup_run: SupRun, args: &[OsString]) -> Result<()> {
init()?;
let channel = sup_run.shared_load.channel;
//let channel = sup_run.shared_load.channel;
let channel = SharedLoad::from(sup_run).channel;
Copy link
Contributor

Choose a reason for hiding this comment

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

send a reference: SharedLoad::from(&sup_run).channel

mwrock
mwrock reviewed Mar 18, 2022
View reviewed changes
components/sup/src/main.rs Outdated

let shared_load = sup_run.shared_load;
//let shared_load = sup_run.shared_load;
let shared_load = hab::cli::hab::svc::SharedLoad::from(sup_run);
Copy link
Contributor

Choose a reason for hiding this comment

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

send a reference: SharedLoad::from(&sup_run)

mwrock
mwrock reviewed Mar 18, 2022
View reviewed changes
components/hab/src/cli/hab/sup.rs Outdated
}

impl From<SupRun> for SharedLoad {
fn from(sup_run: SupRun) -> Self {
Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

The definition can be:

impl From<&SupRun> for SharedLoad {
    fn from(sup_run: &SupRun) -> Self {

mwrock
mwrock reviewed Mar 18, 2022
View reviewed changes
components/hab/src/cli/hab/sup.rs Outdated
password: sup_run.password,
environment: sup_run.environment,
application: sup_run.application,
config_from: sup_run.config_from,
Copy link
Contributor

Choose a reason for hiding this comment

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

Use clone() on these fields: config_from: sup_run.config_from.clone()

@dikshagupta1 dikshagupta1 marked this pull request as ready for review March 28, 2022 14:42
mwrock
mwrock requested changes Mar 28, 2022
View reviewed changes
components/hab/src/cli/hab/sup.rs
pub config_from: Option<PathBuf>,
}

impl From<&SupRun> for SharedLoad {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you moved this to svc.rs then you could avoid having to make some of those members public.

components/hab/src/cli/hab/sup.rs Outdated
bind: sup_run.bind.clone(),
update_condition: sup_run.update_condition.unwrap_or(UpdateCondition::Latest),
binding_mode: sup_run.binding_mode.unwrap_or(BindingMode::Strict),
health_check_interval: sup_run.health_check_interval.unwrap_or(30),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be calling health_check_interval_default instead of the 30.

components/hab/src/command/launcher.rs Outdated
@@ -22,7 +23,7 @@ const LAUNCH_PKG_IDENT: &str = "core/hab-launcher";

pub async fn start(ui: &mut UI, sup_run: SupRun, args: &[OsString]) -> Result<()> {
init()?;
let channel = sup_run.shared_load.channel;
let channel = SharedLoad::from(&sup_run).channel;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could just be sup_run.channel now and no need to transform to a SharedLoad.

mwrock
mwrock requested changes Mar 30, 2022
View reviewed changes
components/hab/src/cli/hab/svc.rs Outdated
@@ -30,6 +30,8 @@ use walkdir::WalkDir;

const DEFAULT_SVC_CONFIG_FILE: &str = "/hab/sup/default/config/svc.toml";
pub const DEFAULT_SVC_CONFIG_DIR: &str = "/hab/sup/default/config/svc";
pub const HEALTH_CHECK_INTERVAL_DEFAULT: u64 = 30;
const HEALTH_CHECK_INTERVAL_STR_DEFAULT: &str = "30";
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary. See comment below.

components/hab/src/cli/hab/svc.rs
@@ -201,7 +203,7 @@ pub struct SharedLoad {
// serialization format. We want to allow the user to simply specify a `u64` to be consistent
// with the CLI, but we cannot change the serialization because the spec file depends on the map
// based format.
#[structopt(long = "health-check-interval", short = "i", default_value = "30")]
#[structopt(long = "health-check-interval", short = "i", default_value = HEALTH_CHECK_INTERVAL_STR_DEFAULT)]
#[serde(default = "health_check_interval_default")]
Copy link
Contributor

Choose a reason for hiding this comment

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

could be : default_value = &HEALTH_CHECK_INTERVAL_DEFAULT.to_string()

@dikshagupta1
Raise usage errors if inappropriate hab svc load options are given in…
cd05d44 
… hab sup run.

Signed-off-by: dikshagupta1 <diksha.gupta@progress.com>
@dikshagupta1 dikshagupta1 merged commit 55375aa into main Mar 31, 2022
@mwrock mwrock deleted the dg/hab-sup-run branch June 2, 2022 15:23
@mwrock mwrock mentioned this pull request Jun 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mwrock mwrock mwrock approved these changes

Assignees

@dikshagupta1 dikshagupta1

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dikshagupta1 @mwrock