feat(azure): add username to azure module config #4323

rjsab · 2022-08-29T12:58:56Z

Description

Allows a user to show the currently logged in account to azure cli by adding $username as an available variable for the Azure Module configuration.

Motivation and Context

Some companies/users work across many Azure tenants or leverage several accounts for separation of environment access, these changes help provide additional visibility to the context of the user you are currently working as.

How Has This Been Tested?

I have tested using MacOS
I have tested using Linux
I have tested using Windows

Checklist:

I have updated the documentation accordingly.
I have updated the tests accordingly.

davidkna

Thanks for the PR! Sorry for only reviewing it now. Please fix the issues I described, so we can merge this.

davidkna · 2022-09-18T15:32:17Z

src/modules/azure.rs

+    let user_name: Option<UserName> = get_azure_user_name(context);
+    if user_name.is_none() {
+        log::info!("Could not find Azure user name");
+        return None;


Since this module can replace multiple things now, please handle missing value in map and remove the unwrap calls there. Since there seems to quite a bit of duplication, please also fetch the user name together with the subscription name.

davidkna · 2022-09-18T15:32:58Z

src/modules/azure.rs

                _ => None,
            })
+            .map(|variable| match variable {


Multiple variables are usually handled with a single closure:

Suggested change

_ => None,

})

.map(|variable| match variable {

davidkna · 2022-09-18T15:36:24Z

src/modules/azure.rs

@@ -194,6 +227,76 @@ mod tests {
        dir.close()
    }

+    #[test]
+    fn user_name_set_correctly() -> io::Result<()> {


Please also add a test with a missing username, but correctly set-up subscription and the reverse.

rjsab · 2022-10-01T03:25:06Z

@davidkna Thank you so much for the review! I apologies for the delay and making the changes. I've taken your feedback and made changes after reading up on Rust and soliciting feedback from a friend more versed in the language.

diskmanti · 2022-10-11T16:04:59Z

Is this going to be merged?

davidkna

Please only define keys in the struct definitions that are needed for the program and allow the program to continue if either the user or subscription name or missing by making those parts Option<String>.
As I mentioned in the last review, as part of this, please allow the module to work even if either the subscription name or user are missing.

rjsab · 2022-10-12T14:13:53Z

Please only define keys in the struct definitions that are needed for the program and allow the program to continue if either the user or subscription name or missing by making those parts Option<String>. As I mentioned in the last review, as part of this, please allow the module to work even if either the subscription name or user are missing.

For a bit of clarity, when you are asking to handle the subscription name or user missing, are you asking for me to handle them missing from the JSON structure entirely, or their values being empty?

davidkna · 2022-10-13T12:10:19Z

For a bit of clarity, when you are asking to handle the subscription name or user missing, are you asking for me to handle them missing from the JSON structure entirely, or their values being empty?

I was thinking of them missing entirely, but empty values would likely be handled the same way.

rjsab · 2022-10-13T19:44:01Z

For a bit of clarity, when you are asking to handle the subscription name or user missing, are you asking for me to handle them missing from the JSON structure entirely, or their values being empty?

I was thinking of them missing entirely, but empty values would likely be handled the same way.

I chose to handle the parse failure on missing keys defined in the struct to allow the program to continue, instead of handling the individual keys as options. I think this would be a more appropriate handling of that situation since a missing name or user key would be more indicative of an issue with the azureProfile config file as a whole, barring any major updates from Microsoft to the format of that file; however, if Microsoft were to change the format of the file, it would likely require updates to this module regardless.

rjsab · 2022-10-27T19:50:56Z

@davidkna - just nudging to see if you had any further recommendations or changes you would like to see. Thanks!

davidkna

I have two suggestions to clean up your code.

Given the supposed strong schema on the JSON-file, not being more flexible here is acceptable. Nevertheless, I don't really think there is much of a reason not to be more fault-tolerant, given that it wouldn't have much of an impact on complexity.

davidkna · 2022-10-29T09:15:50Z

src/modules/azure.rs

+fn load_azure_profile(config_path: &PathBuf) -> Option<AzureProfile> {
+    let json_data = fs::read_to_string(&config_path).expect("Unable to open azureProfile.json");
+    let sanitized_json_data = json_data.strip_prefix('\u{feff}').unwrap_or(&json_data);
+    let azure_profile: Option<AzureProfile> =
+        if let Ok(azure_profile) = serde_json::from_str::<AzureProfile>(sanitized_json_data) {
+            Some(azure_profile)
+        } else {
+            log::info!("Failed to parse azure profile.");
+            None
+        };
+
+    azure_profile
+}


No need to use expect here

Suggested change

fn load_azure_profile(config_path: &PathBuf) -> Option<AzureProfile> {

let json_data = fs::read_to_string(&config_path).expect("Unable to open azureProfile.json");

let sanitized_json_data = json_data.strip_prefix('\u{feff}').unwrap_or(&json_data);

let azure_profile: Option<AzureProfile> =

if let Ok(azure_profile) = serde_json::from_str::<AzureProfile>(sanitized_json_data) {

Some(azure_profile)

} else {

log::info!("Failed to parse azure profile.");

None

};

azure_profile

}

fn load_azure_profile(config_path: &PathBuf) -> Option<AzureProfile> {

let json_data = fs::read_to_string(&config_path).ok()?;

let sanitized_json_data = json_data.strip_prefix('\u{feff}').unwrap_or(&json_data);

if let Ok(azure_profile) = serde_json::from_str::<AzureProfile>(sanitized_json_data) {

Some(azure_profile)

} else {

log::info!("Failed to parse azure profile.");

None

}

}

davidkna · 2022-10-29T09:17:16Z

src/modules/azure.rs

+    if config_path.exists() {
+        let azure_profile: Option<AzureProfile> = load_azure_profile(&config_path);
+
+        if let Some(azure_profile) = azure_profile {
+            let subscription = azure_profile.subscriptions.iter().find_map(|s| {
+                if s.is_default {
+                    Some(s.clone())
+                } else {
+                    None
+                }
+            });
+            subscription
        } else {
            None
        }
-    });
-    if subscription_name.is_some() {
-        subscription_name
    } else {
-        log::info!("Could not find subscription name");
        None
    }


Instead of checking for file existence, just try to open it:

let azure_profile = load_azure_profile(&config_path)?; azure_profile .subscriptions .into_iter() .find(|s| s.is_default)

rjsab · 2022-11-08T19:42:47Z

@davidkna thank you for the recommended changes! I've made those changes, but it appears the builds are failing on a new cargo clippy check for needless borrows. I've corrected those for this module, but build is still failing on the git_commit module.

I wanted to thank you again for your time and patience while working with me on this PR.

allow program to procede if unable to parse azure profile due to missing keys from the JSON structure. remove unused keys from struct Code cleanup with suggestions from PR maintainer Cargo clippy fixes

andytom · 2022-11-27T14:02:48Z

Thank you for your contribution @rjsab and thank you for reviewing @davidkna.

* add username to azure module config * add username to azure module config * formatting with cargo fmt * Handle parse failure on azureProfile.json allow program to procede if unable to parse azure profile due to missing keys from the JSON structure. remove unused keys from struct Code cleanup with suggestions from PR maintainer Cargo clippy fixes

davidkna requested changes Sep 18, 2022

View reviewed changes

davidkna requested changes Oct 11, 2022

View reviewed changes

rjsab force-pushed the azure_module_user_name branch from cb12141 to c96926a Compare October 13, 2022 20:00

davidkna requested changes Oct 29, 2022

View reviewed changes

rjsab force-pushed the azure_module_user_name branch from c96926a to a1a4e12 Compare November 8, 2022 18:46

rjsab force-pushed the azure_module_user_name branch 2 times, most recently from 9a45f78 to 51cf2d4 Compare November 15, 2022 14:14

rjsab added 4 commits November 22, 2022 08:31

add username to azure module config

91557a6

add username to azure module config

a37f257

formatting with cargo fmt

26fe422

Handle parse failure on azureProfile.json

9fe1d31

allow program to procede if unable to parse azure profile due to missing keys from the JSON structure. remove unused keys from struct Code cleanup with suggestions from PR maintainer Cargo clippy fixes

rjsab force-pushed the azure_module_user_name branch from 51cf2d4 to 9fe1d31 Compare November 22, 2022 14:35

davidkna approved these changes Nov 22, 2022

View reviewed changes

andytom merged commit 6e15c00 into starship:master Nov 27, 2022

github-actions bot mentioned this pull request Nov 27, 2022

chore(master): release 1.12.0 #4485

Merged

github-actions bot mentioned this pull request Dec 13, 2022

chore(master): release 1.10.0 #4714

Closed

This was referenced Feb 25, 2023

chore(master): release 1.10.0 #4933

Closed

chore(master): release 1.10.0 #4934

Closed

chore(master): release 1.10.0 #4938

Closed

This was referenced Apr 11, 2023

chore(master): release 1.10.0 #5084

Closed

chore(master): release 1.10.0 #5085

Closed

chore(master): release 1.10.0 #5086

Closed

This was referenced Apr 11, 2023

chore(master): release starship 1.10.0 #5088

Closed

chore(master): release starship 1.10.0 #5089

Closed

chore(master): release 1.14.1 #5090

Merged

chore(master): release 1.10.0 #5091

Closed

chore(master): release 1.10.0 #5101

Closed

github-actions bot mentioned this pull request Jun 6, 2023

chore(master): release 1.16.0 #5224

Closed

github-actions bot mentioned this pull request Jul 30, 2023

chore(master): release 1.17.0 #5348

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(azure): add username to azure module config #4323

feat(azure): add username to azure module config #4323

rjsab commented Aug 29, 2022

davidkna left a comment

davidkna Sep 18, 2022

davidkna Sep 18, 2022

davidkna Sep 18, 2022

rjsab commented Oct 1, 2022

diskmanti commented Oct 11, 2022

davidkna left a comment

rjsab commented Oct 12, 2022

davidkna commented Oct 13, 2022

rjsab commented Oct 13, 2022

rjsab commented Oct 27, 2022

davidkna left a comment

davidkna Oct 29, 2022

davidkna Oct 29, 2022

rjsab commented Nov 8, 2022

andytom commented Nov 27, 2022

feat(azure): add username to azure module config #4323

feat(azure): add username to azure module config #4323

Conversation

rjsab commented Aug 29, 2022

Description

Motivation and Context

How Has This Been Tested?

Checklist:

davidkna left a comment

Choose a reason for hiding this comment

davidkna Sep 18, 2022

Choose a reason for hiding this comment

davidkna Sep 18, 2022

Choose a reason for hiding this comment

davidkna Sep 18, 2022

Choose a reason for hiding this comment

rjsab commented Oct 1, 2022

diskmanti commented Oct 11, 2022

davidkna left a comment

Choose a reason for hiding this comment

rjsab commented Oct 12, 2022

davidkna commented Oct 13, 2022

rjsab commented Oct 13, 2022

rjsab commented Oct 27, 2022

davidkna left a comment

Choose a reason for hiding this comment

davidkna Oct 29, 2022

Choose a reason for hiding this comment

davidkna Oct 29, 2022

Choose a reason for hiding this comment

rjsab commented Nov 8, 2022

andytom commented Nov 27, 2022