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

ListenCtlAddr type #5950

Merged
merged 3 commits into from Jan 4, 2019
Merged

ListenCtlAddr type #5950

merged 3 commits into from Jan 4, 2019

Conversation

mpeck
Copy link
Contributor

@mpeck mpeck commented Dec 10, 2018

Creates a new type ListenCtlAddr in the common crate and uses it to define defaults and env var strings in the clap layer. This also moves the EnvConfig trait to the common crate.

@thesentinels
Copy link
Contributor

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@baumanj
Copy link
Contributor

baumanj commented Dec 10, 2018

Looks like there are some build errors. Let me know if you need any help sorting them out, but I'll hold off on reviewing until they're resolved.

@mpeck
Copy link
Contributor Author

mpeck commented Dec 10, 2018

@baumanj yeah, I missed a couple bits. It should be cleared up now though.

@christophermaier
Copy link
Contributor

Appveyor timed out, so I restarted it.

christophermaier
christophermaier suggested changes Dec 11, 2018
View reviewed changes
Copy link
Contributor

@christophermaier christophermaier left a comment

Choose a reason for hiding this comment

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

Looking good overall, but I think there are still areas where we can tighten things up even further.

components/sup/src/main.rs Outdated Show resolved Hide resolved
components/sup/src/manager/mod.rs Outdated Show resolved Hide resolved
components/sup/src/manager/service/hooks.rs Outdated Show resolved Hide resolved
components/sup/src/manager/service/hooks.rs Outdated Show resolved Hide resolved
components/sup/src/manager/service/mod.rs Show resolved Hide resolved
components/hab/src/main.rs Outdated Show resolved Hide resolved
components/hab/src/main.rs Outdated Show resolved Hide resolved
components/hab/src/main.rs Outdated Show resolved Hide resolved
components/sup/src/main.rs Show resolved Hide resolved
components/common/src/listen_ctl_addr.rs Outdated Show resolved Hide resolved
baumanj
baumanj suggested changes Dec 11, 2018
View reviewed changes
Copy link
Contributor

@baumanj baumanj left a comment

Choose a reason for hiding this comment

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

I haven't finished looking at the entire PR, but since I had some fairly big suggestions for re-organizing, I wanted to get you that feedback sooner. I'll keep reviewing in the meantime, but let me know if you have any questions.

components/common/src/lib.rs Outdated Show resolved Hide resolved
components/common/src/listen_ctl_addr.rs Outdated Show resolved Hide resolved
baumanj
baumanj reviewed Dec 11, 2018
View reviewed changes
components/hab/src/main.rs Outdated Show resolved Hide resolved
baumanj
baumanj reviewed Dec 11, 2018
View reviewed changes
components/sup/src/main.rs Outdated Show resolved Hide resolved
@mpeck mpeck force-pushed the peck/listen-ctl branch 9 times, most recently from dd7a24f to 3c31952 Compare December 14, 2018 18:37
@mpeck mpeck force-pushed the peck/listen-ctl branch 4 times, most recently from 85e90b9 to ba52b88 Compare January 3, 2019 19:53
christophermaier
christophermaier suggested changes Jan 3, 2019
View reviewed changes
Copy link
Contributor

@christophermaier christophermaier left a comment

Choose a reason for hiding this comment

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

A few fixes, then I think we can put this to bed.

components/hab/src/main.rs Show resolved Hide resolved
components/hab/src/main.rs Show resolved Hide resolved
christophermaier
christophermaier suggested changes Jan 4, 2019
View reviewed changes
components/sup/src/main.rs
Ok(default)
},
|v| v.parse(),
)?,
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it'd be good to use unwrap_or_default on these... I don't know that all the println calls are adding much here.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was my suggestion. I'd change them to error! since if we get there, it implies we've broken something in the clap code, but I'd rather not panic for something that's not so dire.

Copy link
Contributor Author

@mpeck mpeck Jan 4, 2019 •
edited

Choose a reason for hiding this comment

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

I thought it would be nice to let the user know that the behavior might not be exactly as expected. Though technically they should never get to that point. I can drop the println calls but I think we still need to use map_or_else since the default value is already of the correct type but the unwrapped value would be a string.

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 changed them to error!. I'm happy to remove them if preferred though.

@mpeck mpeck force-pushed the peck/listen-ctl branch 2 times, most recently from 30fa2dc to 7e84e4f Compare January 4, 2019 16:39
Matthew Peck added 3 commits January 4, 2019 16:09
add listen ctl type
7b166e2 
Signed-off-by: Matthew Peck <mpeck@chef.io>
rename defaults to cli_defaults
b7c0ef9 
Signed-off-by: Matthew Peck <mpeck@chef.io>
Set remaning ManagerConfig values in the declaration block
5462e48 
Signed-off-by: Matthew Peck <mpeck@chef.io>
@mpeck mpeck merged commit f3f0fad into master Jan 4, 2019
@mpeck mpeck deleted the peck/listen-ctl branch January 4, 2019 23:22
chef-ci added a commit that referenced this pull request Jan 4, 2019
@chef-ci
Update CHANGELOG.md with details from pull request #5950
b921b0c 
Obvious fix; these changes are the result of automation not creative thinking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christophermaier christophermaier christophermaier approved these changes

@baumanj baumanj baumanj approved these changes

@chefsalim chefsalim Awaiting requested review from chefsalim

@eeyun eeyun Awaiting requested review from eeyun

@fnichol fnichol Awaiting requested review from fnichol

@raskchanky raskchanky Awaiting requested review from raskchanky

@reset reset Awaiting requested review from reset

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mpeck @thesentinels @baumanj @christophermaier