builder origin rbac subcommand #7841

jeremymv2 · 2020-07-28T20:07:45Z

Closes #7599
Companion to habitat-sh/builder#1431

Although there was a lot more potential structopt work to do, I had to stop myself and stash that effort as the blast radius became very broad (ie. AuthToken, Origin and BldrURL touch 90% of CLI commands).
To handle the remaining structopt work that will allow us to be more streamlined, I created #7866

Signed-off-by: Jeremy J. Miller [email protected]

eeyun · 2020-07-29T14:25:54Z

Code looks good, but it does appear to have broken some unit tests. Otherwise its 👍 from me.

jeremymv2 · 2020-07-29T15:43:29Z

@eeyun the unit tests are fixed up now.

davidMcneil

Very nice; a great addition!

components/builder-api-client/src/lib.rs

components/hab/src/cli/hab/origin.rs

components/hab/src/cli.rs

components/hab/src/cli/hab/origin.rs

components/hab/src/main.rs

Signed-off-by: Jeremy J. Miller <[email protected]>

…messsaging Signed-off-by: Jeremy J. Miller <[email protected]>

Signed-off-by: Jeremy J. Miller <[email protected]>

components/builder-api-client/src/error.rs

Signed-off-by: Jeremy J. Miller <[email protected]>

components/core/src/util/text_render.rs

davidMcneil · 2020-08-11T16:32:05Z

components/builder-api-client/src/lib.rs

-fn tabify(mut tw: TabWriter<Vec<u8>>, s: &str) -> Result<String> {
-    write!(&mut tw, "{}", s)?;
-    tw.flush()?;
-    String::from_utf8(tw.into_inner().expect("TABWRITER into_inner")).map_err(|e| {


Because this function already returns a Result, could we remove this expect in favor of propagating the Err?

Handled in 78f9d5e

components/core/src/origin.rs

components/core/Cargo.toml

components/hab/src/cli/hab/util.rs

davidMcneil · 2020-08-11T17:15:43Z

components/hab/src/cli/hab/util.rs

+                       })
+}
+
+pub fn bldr_url_from_args_env_load_or_default(opt: Option<Url>) -> String {


It is a bummer we lose type information here (ie Url to String). Could we make this return a Url?

The config file should probably require bldr_url be a Url, correct?

The function now returns a Result<Url> type instead of String. Take a look and see if what's there now looks agreeable. 3568a83

components/hab/src/cli/hab/util.rs

Signed-off-by: Jeremy J. Miller <[email protected]>

davidMcneil · 2020-08-12T13:35:29Z

components/core/src/origin.rs

+pub const ORIGIN_MEMBER_ROLE_READONLY_MEMBER: &str = "readonly_member";
+pub const ORIGIN_MEMBER_ROLE_MEMBER: &str = "member";
+pub const ORIGIN_MEMBER_ROLE_MAINTAINER: &str = "maintainer";
+pub const ORIGIN_MEMBER_ROLE_ADMINISTRATOR: &str = "administrator";
+pub const ORIGIN_MEMBER_ROLE_OWNER: &str = "owner";


No need to change anything, but I am personally a fan of asciating constants like this with the type. So something like:

impl OriginMemberRole { const READONLY_MEMBER: &'static str = "readonly_member"; const MEMBER: &'static str = "member"; const MAINTAINER: &'static str = "maintainer"; const ADMINISTRATOR: &'static str = "administrator"; const OWNER: &'static str = "owner"; }

You can then access them with OriginMemberRole::READONLY_MEMBER

Totally agree. Adjusted in 9aee65a

I like your change. Although, it was not quite what I had in mind 😄. My intention was to scope the constants within the OriginMemberRole so they should be defined in an impl OriginMemberRole { } block and accessed with OriginMemberRole::READONLY_MEMBER. But I dont think there is any need to change anything for this case.

ahh yes. I'll make it so.

I've modified in 78f9d5e

…|show functions Signed-off-by: Jeremy J. Miller <[email protected]>

Signed-off-by: Jeremy J. Miller <[email protected]>

jeremymv2 · 2020-08-12T15:30:44Z

@davidMcneil I believe I've taken care of all the feedback items thus far. Care to sanity check me? ❤️

davidMcneil · 2020-08-12T16:01:04Z

components/hab/src/cli/hab/origin.rs

+    /// The role name to enforce for the member account [values: readonly_member, member,
+    /// maintainer, administrator, owner]


We could use #[structopt(possible_values = &[OriginMemberRole::READONLY_MEMBER ...])] which will automatically display the possible values and has better traceability should we ever add an OriginMemberRole` variant.

This is a nice capability. Added in 78f9d5e

davidMcneil · 2020-08-12T16:15:05Z

components/core/src/util/text_render.rs

+    let res = tw.into_inner();
+    if res.is_err() {
+        return Err(Error::TabWriterIntoInnerFailed("Unable to flush \
+                                                    tabwriter buffer to \
+                                                    inner."
+                                                           .to_string()));
+    }
+    let inner = res.unwrap();


I dont think we should have a custom TabWriterIntoInnerFailed type. Instead we could use the io::Error that is part of tabwriter::IntoInnerError

We could use:

let inner = tw.into_inner().map_err(|e| { IoError::new(e.error().kind(), "Unable to flush tabwriter buffer to inner.") })?;

This preserves the io::Error kind, prevents us from having to create a new type, and avoids an unwrap.

Modified as such in 78f9d5e

davidMcneil · 2020-08-12T16:15:45Z

components/core/src/error.rs

@@ -340,6 +347,7 @@ impl fmt::Display for Error {
            Error::GetExitCodeProcessFailed(ref e) => e.to_string(),
            Error::CreateToolhelp32SnapshotFailed(ref e) => e.to_string(),
            Error::WaitForSingleObjectFailed(ref e) => e.to_string(),
+            Error::TabWriterIntoInnerFailed(ref e) => e.to_string(),


See other comments. I dont think this type should be needed.

Removed in 78f9d5e

davidMcneil · 2020-08-12T16:19:43Z

components/hab/src/error.rs

@@ -48,6 +48,7 @@ pub enum Error {
    HabitatCore(hcore::Error),
    // Boxed due to clippy::large_enum_variant
    HandlebarsRenderError(Box<handlebars::TemplateRenderError>),
+    InvalidUrl(String),


I dont think we should add this variant. See other comments.

Variant removed in 78f9d5e

davidMcneil · 2020-08-12T16:21:33Z

components/hab/src/cli/hab/util.rs

+        Url::parse(&bldr_url_from_env_load_or_default()).map_err(|e| {
+                                                            Error::InvalidUrl(e.to_string())
+                                                        })


Instead of mapping the error here, because this is the only possible error the function can return can we change the Error type the function returns?

removed the error mapping in 78f9d5e

davidMcneil · 2020-08-12T16:22:25Z

components/hab/src/cli/hab/util.rs

+    pub inner: Origin,
+}
+
+pub fn bldr_url_from_env_load_or_default() -> String {


This could be private.

Changed to private in 78f9d5e

davidMcneil · 2020-08-12T16:26:38Z

components/hab/src/cli/hab/util.rs

+        match henv::var(AUTH_TOKEN_ENVVAR) {
+            Ok(v) => Ok(v),
+            Err(_) => {
+                config::load()?.auth_token.ok_or_else(|| {
+                                              Error::ArgumentError("No auth token specified. \
+                                                                    Please check that you have \
+                                                                    specified a valid Personal \
+                                                                    Access Token with:  -z, \
+                                                                    --auth <AUTH_TOKEN>"
+                                                                                        .into())
+                                          })
+            }


I think this could use or_else

I believe this has to be ok_or_else since config::load is returning an option for auth_token. If I change to or_else clippy says:

error[E0308]: mismatched types --> components/hab/src/cli/hab/util.rs:90:17 | 83 | pub fn bldr_auth_token_from_args_env_or_load(opt: Option<String>) -> Result<String, Error> { | --------------------- expected `std::result::Result<std::string::String, error::Error>` because of return type ... 90 | / config::load()?.auth_token.or_else(|| { 91 | | Error::ArgumentError("No auth token specified. \ 92 | | Please check that you have \ 93 | | specified a valid Personal \ ... | 96 | | .into()) 97 | | }) | |____________________________________________^ expected enum `std::result::Result`, found enum `std::option::Option` | = note: expected enum `std::result::Result<std::string::String, error::Error>` found enum `std::option::Option<std::string::String>`

I need to return an result so that the CLI marshaling can error if no auth token is found and one is required. We need to figure out how to handle this natively in the future (loading from args, ENV, OR cli.toml. See: #7866

For now we need to return a Result so that we can error appropriately before calling the start function of the command:
let auth_token = bldr_auth_token_from_args_env_or_load(r.auth_token.value)?;

Yeah, sorry I was not clear. I meant this part match henv::var(AUTH_TOKEN_ENVVAR) we could probably avoid the need to match on the Ok just to wrap it back up in Ok, but it would not really buy us that much.

davidMcneil · 2020-08-12T16:28:34Z

components/hab/src/command/origin/rbac/show_role.rs

+                }
+            } else {
+                ui.status(Status::Discovering, "origin member role".to_string())?;
+                println!("Member {} has the '{}' role in the {} origin.",


Should this be a println? Maybe a ui.status or debug! instead?

Added ui.status in 78f9d5e

Signed-off-by: Jeremy J. Miller <[email protected]>

davidMcneil

Looks great, thanks!

jeremymv2 requested review from christophermaier, davidMcneil and eeyun as code owners July 28, 2020 20:07

chef-expeditor bot assigned jeremymv2 Jul 28, 2020

jeremymv2 force-pushed the jeremymv2/rbac_subcommand branch from da16a6a to 5c56610 Compare July 28, 2020 20:27

jeremymv2 added the Type:Feature PRs that add a new feature label Jul 29, 2020

davidMcneil suggested changes Jul 30, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

jeremymv2 added 11 commits August 11, 2020 09:16

builder origin rbac operations

ed81c10

Signed-off-by: Jeremy J. Miller <[email protected]>

a rebase merge conflict fix and some improved error handling and cli …

73db411

…messsaging Signed-off-by: Jeremy J. Miller <[email protected]>

hab client subcommand for origin rbac operations

520ca21

Signed-off-by: Jeremy J. Miller <[email protected]>

correcting typo in error message

39092e8

Signed-off-by: Jeremy J. Miller <[email protected]>

clippy issues

f9ec5a7

Signed-off-by: Jeremy J. Miller <[email protected]>

adding missing derive ConfigOpt

0c8864c

Signed-off-by: Jeremy J. Miller <[email protected]>

fix rbac subcommand help to match configopt

640e2ed

Signed-off-by: Jeremy J. Miller <[email protected]>

add missing discription for set configopt variant

7016aa0

Signed-off-by: Jeremy J. Miller <[email protected]>

truing up structops with cli args and options

dd3c979

Signed-off-by: Jeremy J. Miller <[email protected]>

an implementation with structopt that is suitable

7b1f484

Signed-off-by: Jeremy J. Miller <[email protected]>

fix rebase merge conflict

4c94bd8

Signed-off-by: Jeremy J. Miller <[email protected]>

jeremymv2 force-pushed the jeremymv2/rbac_subcommand branch from 9472ea6 to 4c94bd8 Compare August 11, 2020 13:37

jeremymv2 added 3 commits August 11, 2020 09:44

adding origin and text_render utility module in core

6381ea0

Signed-off-by: Jeremy J. Miller <[email protected]>

moving text render traits into core::util::text_render

787dd7d

Signed-off-by: Jeremy J. Miller <[email protected]>

removing WIP intermediary file artifact

43ca9dd

Signed-off-by: Jeremy J. Miller <[email protected]>

jeremymv2 commented Aug 11, 2020

View reviewed changes

components/builder-api-client/src/error.rs Outdated Show resolved Hide resolved

removing unused Error entry for the builder-api-client

aff6e0e

Signed-off-by: Jeremy J. Miller <[email protected]>

davidMcneil suggested changes Aug 11, 2020

View reviewed changes

jeremymv2 added 2 commits August 11, 2020 18:16

origin member role constants

633e750

Signed-off-by: Jeremy J. Miller <[email protected]>

adding TabWriterIntoInnerFailed error variant

6efd8ad

Signed-off-by: Jeremy J. Miller <[email protected]>

validator for parsing Url from args, environment, toml or default

3568a83

Signed-off-by: Jeremy J. Miller <[email protected]>

davidMcneil reviewed Aug 12, 2020

View reviewed changes

jeremymv2 added 2 commits August 12, 2020 10:42

wire up type primitives all the way to backend builder-api-client set…

80dffb7

…|show functions Signed-off-by: Jeremy J. Miller <[email protected]>

fixup constants and adjust help text

9aee65a

Signed-off-by: Jeremy J. Miller <[email protected]>

davidMcneil suggested changes Aug 12, 2020

View reviewed changes

moving constants into impl OriginMemberRole and better error handling

78f9d5e

Signed-off-by: Jeremy J. Miller <[email protected]>

davidMcneil approved these changes Aug 12, 2020

View reviewed changes

jeremymv2 merged commit 020eb18 into master Aug 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builder origin rbac subcommand #7841

builder origin rbac subcommand #7841

jeremymv2 commented Jul 28, 2020 •

edited

Loading

eeyun commented Jul 29, 2020

jeremymv2 commented Jul 29, 2020

davidMcneil left a comment

This comment has been minimized.

davidMcneil Aug 11, 2020

jeremymv2 Aug 12, 2020 •

edited

Loading

davidMcneil Aug 11, 2020

davidMcneil Aug 11, 2020

jeremymv2 Aug 12, 2020 •

edited

Loading

davidMcneil Aug 12, 2020

jeremymv2 Aug 12, 2020

davidMcneil Aug 12, 2020

jeremymv2 Aug 12, 2020

jeremymv2 Aug 12, 2020

jeremymv2 commented Aug 12, 2020

davidMcneil Aug 12, 2020

jeremymv2 Aug 12, 2020

davidMcneil Aug 12, 2020

jeremymv2 Aug 12, 2020

davidMcneil Aug 12, 2020

jeremymv2 Aug 12, 2020

davidMcneil Aug 12, 2020

jeremymv2 Aug 12, 2020

davidMcneil Aug 12, 2020

jeremymv2 Aug 12, 2020

davidMcneil Aug 12, 2020

jeremymv2 Aug 12, 2020

davidMcneil Aug 12, 2020

jeremymv2 Aug 12, 2020

davidMcneil Aug 12, 2020

davidMcneil Aug 12, 2020

jeremymv2 Aug 12, 2020

davidMcneil left a comment

		/// The role name to enforce for the member account [values: readonly_member, member,
		/// maintainer, administrator, owner]

builder origin rbac subcommand #7841

builder origin rbac subcommand #7841

Conversation

jeremymv2 commented Jul 28, 2020 • edited Loading

eeyun commented Jul 29, 2020

jeremymv2 commented Jul 29, 2020

davidMcneil left a comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

jeremymv2 Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremymv2 Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremymv2 commented Aug 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidMcneil left a comment

Choose a reason for hiding this comment

jeremymv2 commented Jul 28, 2020 •

edited

Loading

jeremymv2 Aug 12, 2020 •

edited

Loading

jeremymv2 Aug 12, 2020 •

edited

Loading