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

feat(iam): support assume role #691

Merged
merged 7 commits into from
Jan 4, 2024

Conversation

JBOClara
Copy link
Contributor

@JBOClara JBOClara commented Nov 30, 2023

Fix #691

Copy link
Contributor

@adejanovski adejanovski left a comment

Choose a reason for hiding this comment

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

Hi @JBOClara,

a few things here:

  • There's a lot of noise in this PR due to reformatting of unmodified code, most probably due to your IDE settings. Could you revert these changes so that it's clear what was modified by this PR specifically?
  • This comment suggests that there's a simpler solution which would be to construct the client without passing credentials if none are provided. That seems like an interesting option but is not what's done in your PR. What's your opinion on this?

@azman0101
Copy link

Hi @JBOClara,

a few things here:

  • There's a lot of noise in this PR due to reformatting of unmodified code, most probably due to your IDE settings. Could you revert these changes so that it's clear what was modified by this PR specifically?
  • This comment suggests that there's a simpler solution which would be to construct the client without passing credentials if none are provided. That seems like an interesting option but is not what's done in your PR. What's your opinion on this?

Ok, I'll review the formatting.

About the simpler solution, I'm currently implementing it.

I have a hard time mocking the token file /var/run/secrets/eks.amazonaws.com/serviceaccount/token to cover my new code.

@adejanovski
Copy link
Contributor

Hi @JBOClara,
a few things here:

  • There's a lot of noise in this PR due to reformatting of unmodified code, most probably due to your IDE settings. Could you revert these changes so that it's clear what was modified by this PR specifically?
  • This comment suggests that there's a simpler solution which would be to construct the client without passing credentials if none are provided. That seems like an interesting option but is not what's done in your PR. What's your opinion on this?

Ok, I'll review the formatting.

About the simpler solution, I'm currently implementing it.

I have a hard time mocking the token file /var/run/secrets/eks.amazonaws.com/serviceaccount/token to cover my new code.

I'm confused, you're using two separate github accounts, right? 🙂
Let me know if we can help with your current mocking issues to get this moving forward.

@JBOClara
Copy link
Contributor Author

I'm confused, you're using two separate github accounts, right? 🙂
Let me know if we can help with your current mocking issues to get this moving forward.

Sorry for the confusion, I'll make sure not to make the mistake again.

I will push my changes to discuss this topic.

@adejanovski
Copy link
Contributor

Sorry for the confusion, I'll make sure not to make the mistake again.

I will push my changes to discuss this topic.

No worries, thanks!

@JBOClara JBOClara force-pushed the feat/support-assume-role branch from d4bb758 to 2a0afe0 Compare December 20, 2023 16:58
@JBOClara JBOClara marked this pull request as draft December 20, 2023 16:58
@JBOClara JBOClara force-pushed the feat/support-assume-role branch 6 times, most recently from 3dcd800 to 82cc1ae Compare December 20, 2023 17:11
Copy link
Contributor

@adejanovski adejanovski left a comment

Choose a reason for hiding this comment

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

Changes look good and I was able to successfully test them using a role attached to an ec2 instance 👍

Issue: CI is failing due to formatting issues detected by flake8. See here.
Once these are fixed and CI passes, we'll be good for a merge.
Thanks!

@JBOClara JBOClara force-pushed the feat/support-assume-role branch 4 times, most recently from 8a9a189 to df4d929 Compare January 4, 2024 11:13
@JBOClara JBOClara force-pushed the feat/support-assume-role branch 5 times, most recently from 4f73326 to 373866d Compare January 4, 2024 11:19
@JBOClara JBOClara force-pushed the feat/support-assume-role branch from 373866d to 847c470 Compare January 4, 2024 11:21
@JBOClara JBOClara marked this pull request as ready for review January 4, 2024 11:21
Copy link

sonarqubecloud bot commented Jan 4, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@adejanovski adejanovski left a comment

Choose a reason for hiding this comment

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

Awesome stuff, thanks @JBOClara 🙏

@adejanovski adejanovski merged commit 50e1dad into thelastpickle:master Jan 4, 2024
29 of 31 checks passed
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.

S3: medusa getting stuck indefinitely while fetching uploaded object
3 participants