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 aws irsa auth #1021

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

smcaine
Copy link

@smcaine smcaine commented Oct 9, 2024

Adding this flag to use the service account iam role, for when using eks: https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts-minimum-sdk.html

Using aws access keys is bad practice and typically prohibited in most production environments.

This relates to issue i raised #949

@smcaine smcaine requested a review from a team as a code owner October 9, 2024 10:00
Copy link

@paul-carlton paul-carlton left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -102,6 +103,7 @@ def __init__(
self,
url,
encryption=None,
aws_irsa=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

please relocate this new argument at the end of the argument list to preserve the original signature @smcaine

@gcalacoci
Copy link
Contributor

@smcaine thanks for your contribution.

I've put a suggestion in the code during my review of the pr.

The code seems correct but your patch has broken the Unit Tests for Barman, because of this we cannot merge the PR as it is.

Could you fix the failing tests?
Would also be of help for you to add some specific Unit Tests for the code you introduced.

FYI we are working on some contribution guidelines for PRs and what I'm asking will be part of this new documentation section.

Thanks.

@martinmarques
Copy link
Contributor

Hi Stuart, as mentioned before, your PR is failing for tox tests. Can you check these and push a fix that makes them pass?

https://github.com/EnterpriseDB/barman/actions/runs/11720727101/job/32805402247?pr=1021

There are also flack errors that need addressing:

https://github.com/EnterpriseDB/barman/actions/runs/11720727101/job/32805400982?pr=1021

@smcaine
Copy link
Author

smcaine commented Dec 4, 2024

Hi Stuart, as mentioned before, your PR is failing for tox tests. Can you check these and push a fix that makes them pass?

https://github.com/EnterpriseDB/barman/actions/runs/11720727101/job/32805402247?pr=1021

There are also flack errors that need addressing:

https://github.com/EnterpriseDB/barman/actions/runs/11720727101/job/32805400982?pr=1021

yes, apologies, i have not been able to pick this up, will start running through these and add tests for the work done in the PR

@smcaine smcaine marked this pull request as draft December 4, 2024 15:33
@smcaine smcaine marked this pull request as ready for review December 9, 2024 10:56
@smcaine
Copy link
Author

smcaine commented Dec 9, 2024

Can these tests be re-run? Hopefully I've covered everything. Ive just revised PR so its clear this is just for EKS and IAM role Service accounts

@smcaine smcaine requested a review from gcalacoci December 18, 2024 12:42
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.

4 participants