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.

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

Add interactive prompt for AWS profile name #111

Merged
merged 9 commits into from
Feb 21, 2023

Conversation

opis-mark
Copy link
Contributor

@opis-mark opis-mark commented Feb 3, 2023

Description

Ask the user what name to give the AWS credential profile after the role is selected.

Related Issue

#110

Motivation and Context

Currently the AWS profile is defaulted to the IAM role name but this is not specific enough for my needs as I have access to multiple accounts with matching role names. I prefix my profile names with the AWS account name so that I can have confidence that I am working in the correct environment.

How Has This Been Tested?

I ran through the interactive prompts and the output worked as expected. I then tested the local profile and was able to successfully execute commands with the AWS cli using the generated credentials. More testing should be done to validate expected behavior when environment variables, saved config files, or command line options which should exclude the prompt from being displayed.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@opis-mark
Copy link
Contributor Author

@pcmxgti I don't think I have the ability to assign team members to this PR. Is that something you can do?

@opis-mark opis-mark changed the title Add feature to prompt user for profile name Add interactive prompt for AWS profile name Feb 6, 2023
@pcmxgti pcmxgti requested review from sevignyj and removed request for sevignyj February 6, 2023 15:17
@opis-mark opis-mark force-pushed the feature/profile-name-prompt branch from a07fd4b to f1a9141 Compare February 16, 2023 04:13
@opis-mark
Copy link
Contributor Author

Sorry for all the bad commits and spam. I finally got my local dev environment working and am able to run tox before pushing commit to github. Future PRs should be smoother :) I think this is ready for review, @sevignyj.

@sevignyj
Copy link
Contributor

@opis-mark No worries about the commits, keep them coming, it is much appreciated! I will give this more time over the weekend. Thanks!!

Copy link
Contributor

@sevignyj sevignyj left a comment

Choose a reason for hiding this comment

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

Hi @opis-mark , Thank you so much for this PR, it is a nice feature.

If you could make some minor changes - basically to only call the set method from tool.py, and name change, as:

diff --git a/tests/unit_test.py b/tests/unit_test.py
index 87ae1de..0a83d29 100644
--- a/tests/unit_test.py
+++ b/tests/unit_test.py
@@ -1182,15 +1182,15 @@ def test_process_interactive_input(mocker):
         ("role_name", "role_name", "role_name"),
     ],
 )
-def test_get_profile_name(mocker, default, submit, expected):
+def test_get_interactive_profile_name(mocker, default, submit, expected):
     """Test getting the AWS profile name form user input."""
     from tokendito import user
 
     mocker.patch("tokendito.user.input", return_value=submit)
-    assert user.get_profile_name(default) == expected
+    assert user.get_interactive_profile_name(default) == expected
 
 
-def test_get_profile_name_invalid_input(monkeypatch):
+def test_get_interactive_profile_name_invalid_input(monkeypatch):
     """Test reprompting the AWS profile name form user on invalid input."""
     from tokendito import user
 
@@ -1200,7 +1200,7 @@ def test_get_profile_name_invalid_input(monkeypatch):
     # using lambda statement for mocking
     monkeypatch.setattr("builtins.input", lambda name: next(inputs))
 
-    assert user.get_profile_name("role_name") == "valid"
+    assert user.get_interactive_profile_name("role_name") == "valid"
 
 
 @pytest.mark.parametrize(
diff --git a/tokendito/tool.py b/tokendito/tool.py
index 6494b8d..bd2e150 100644
--- a/tokendito/tool.py
+++ b/tokendito/tool.py
@@ -63,11 +63,7 @@ def cli(args):
         )
         sys.exit(1)
 
-    profile_name = config.aws["profile"]
-    if profile_name is None or profile_name == "":
-        profile_name = user.get_profile_name(role_name)
-
-    user.set_profile_name(config, profile_name)
+    user.set_profile_name(config, role_name)
 
     user.set_local_credentials(
         response=role_response,
diff --git a/tokendito/user.py b/tokendito/user.py
index 8432896..b82e84b 100644
--- a/tokendito/user.py
+++ b/tokendito/user.py
@@ -864,7 +864,7 @@ def get_password():
     return res
 
 
-def get_profile_name(default):
+def get_interactive_profile_name(default):
     """Get AWS profile name from user.
 
     :return: string with sanitized value, or the default string.
@@ -886,17 +886,14 @@ def get_profile_name(default):
     return res
 
 
-def set_profile_name(config_obj, name):
+def set_profile_name(config_obj, role_name):
     """Set AWS Role alias name based on user preferences.
 
     :param config: Config object.
-    :param name: Role name. Defaults to the string "default"
     :return: Config object.
     """
-    if name is None or name == "":
-        name = "default"
-    if config_obj.aws["profile"] is None:
-        config_obj.aws["profile"] = str(name)
+    if config.aws["profile"] is None or config.aws["profile"] == "":
+        config.aws["profile"] = get_interactive_profile_name(role_name)
 
     return config_obj
 

Thank you very much!

@opis-mark
Copy link
Contributor Author

@sevignyj - I've got your changes made locally but I could use a recommendation for the user.set_profile_name tests. It's not obvious to me if role_name can ever be None or empty as it should be coming from AWS and I don't think those are possible values. Furthermore, I will probably mock the calls to in those tests to get_interactive_profile_name as that'll be tested separately.

@opis-mark opis-mark requested a review from sevignyj February 21, 2023 04:59
Copy link
Contributor

@sevignyj sevignyj left a comment

Choose a reason for hiding this comment

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

Thank you @opis-mark , looks great!

@sevignyj sevignyj merged commit b26d2fd into dowjones:main Feb 21, 2023
@opis-mark opis-mark deleted the feature/profile-name-prompt branch February 21, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants