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

Try refreshing tokens as needed #73

Merged
merged 5 commits into from
Dec 9, 2023
Merged

Conversation

matan129
Copy link
Contributor

@matan129 matan129 commented Nov 30, 2023

Should solve #71.

Tested locally with sso_session setup, but I'd appreciate verification with other setups.

Edit: Something is wrong when sso_session is combined with source_profile, I'll fix and update.

@matan129
Copy link
Contributor Author

matan129 commented Nov 30, 2023

Hi @pcolmer , it seems that the CI fails due to unrelated reasons (Python 3.6/7 not supported anymore by Github actions).
The unit tests pass on my fork here.

@matan129 matan129 marked this pull request as draft November 30, 2023 17:02
@matan129 matan129 marked this pull request as ready for review November 30, 2023 18:20
@pcolmer
Copy link
Contributor

pcolmer commented Nov 30, 2023

@matan129 thank you very much for this contribution. Does the user need to set up their AWS config in any particular way for this to work? (I'm just thinking about the README file and whether some guidance/instructions might be needed)

@matan129
Copy link
Contributor Author

Nope. This just make aws2-wrap support already-valid configs which utilize sso_session (which implies refreshable tokens), with or without source_profile setup.

Other configs should keep their normal non-refreshy behavior. Having said that, I didn't check configs without sso_session so I can't be sure I haven't messed up other use cases.

@matan129
Copy link
Contributor Author

matan129 commented Dec 6, 2023

@pcolmer anything else I should do?

@pcolmer
Copy link
Contributor

pcolmer commented Dec 9, 2023

@matan129

anything else I should do?

No, I don't think so. My apologies for being "absent" on this ... workload has been through the roof lately.

I've tried running the updated code on my machine against both types of profiles and it is working fine.

Thank you very much for this contribution. It is really appreciated.

@pcolmer pcolmer merged commit fc2dc92 into linaro-its:master Dec 9, 2023
3 checks passed
@pcolmer
Copy link
Contributor

pcolmer commented Dec 9, 2023

@matan129 I'm trying to add the function docstrings and I'm unclear what parent_profile_name refers to. Can you please explain it so I can add the comments to the code? Thanks.

@pcolmer
Copy link
Contributor

pcolmer commented Dec 9, 2023

@matan129 I've tried to sort out some of the parameter types as well. I'm a bit concerned that although make test succeeeds, there are some type incompatibilities that seem to mostly focus on ProfileDef.

It could be that the tests are too "lazy" in their approach, for example passing "jim" as a value to a parameter which is now defined as type ProfileDef.

The purpose of the tests is to try to ensure that a new release of aws2-wrap doesn't break compatibility with other code that calls the inner functions. So, as make test succeeds, I'm not overly concerned but if there are further changes you can make to try to deal with the errors/warnings getting flagged up in test_aws2wrap.py, it would be appreciated.

@matan129
Copy link
Contributor Author

matan129 commented Dec 9, 2023

@matan129 I'm trying to add the function docstrings and I'm unclear what parent_profile_name refers to. Can you please explain it so I can add the comments to the code? Thanks.

Consider this AWS config:

[profile a]
source_profile = b
...

[profile b]
...

Then I'd consider a to be b's parent.

Regarding type issue, I'm not sure what you mean. I don't see any warning when running make mypy test.

@pcolmer
Copy link
Contributor

pcolmer commented Dec 10, 2023

@matan129

Regarding type issue, I'm not sure what you mean. I don't see any warning when running make mypy test.

I'm not seeing it from mypy test. I'm seeing it in VS Code when I open test_aws2wrap.py. Some of the issues have arisen because I went through the main script changing the type for the profile name from str to ProfileDef (which is something I think you added). So, for example, line 41 in the test script says this:

_ = aws2wrap.get_role_credentials(self.PROFILE)

and VS Code warns:

Argument of type "dict[str, str]" cannot be assigned to parameter "profile" of type "ProfileDef" in function "get_role_credentials"
  "dict[str, str]" is incompatible with "ProfileDef"
    Type parameter "_VT@dict" is invariant, but "str" is not the same as "str | Dict[str, Any]"
    Consider switching from "dict" to "Mapping" which is covariant in the value type

As I said, it isn't breaking the tests per se ... but one of the reasons why typing and the tests were introduced were to ensure that the functions didn't break compatibility with anything else using them. I think that some of the incompatibilities are showing up because of lazy tests but I can't be entirely sure - it has been a while since they were written and I'd have to look back over the git history to see who wrote them :)

(Probably me, knowing my luck!)

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