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

Dependency - Update minio-go to v7.0.70 #7335

Merged
merged 2 commits into from
May 6, 2024
Merged

Conversation

eqfarhad
Copy link
Contributor

@eqfarhad eqfarhad commented May 3, 2024

fix issue: #7157

Bumping the Minio-go dependency version to the v7.0.70 as it contains support for EKS Pod Identity

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Bump the minio-go version to v7.0.70

@eqfarhad eqfarhad force-pushed the main branch 2 times, most recently from efd7c0e to bc19a21 Compare May 3, 2024 23:21
@eqfarhad eqfarhad marked this pull request as ready for review May 3, 2024 23:34
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.
However, I think it is still unclear to me how to use the EKS Pod Identity feature. Do we need to update docs somewhere for instructions?

@eqfarhad
Copy link
Contributor Author

eqfarhad commented May 5, 2024

I think it is still unclear to me how to use the EKS Pod Identity feature.

IMO it's more on AWS side; the docs are available on their side and well explained how it works or how to use Pod Identity feature instead of IAM Roles for service accounts (IRSA) in EKS.

in case of Thanos IIUC based on the s3 pkg and your comment we are relying on Minio-go, hence they had to push the update first to support it;
Which they did and here in the update for minio-go, we can see that now beside the other env variables it also checks the AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE that's a new env var provided by Pod Identity feature

Do we need to update docs somewhere for instructions?

I did a quick search in the docs, and maybe we can add it here, wdyt?

@yeya24
Copy link
Contributor

yeya24 commented May 6, 2024

Cool, thanks for the comment. I think I am ok to proceed with this pr without updating docs. We don't have doc for AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE as well

yeya24
yeya24 previously approved these changes May 6, 2024
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks

@yeya24
Copy link
Contributor

yeya24 commented May 6, 2024

There is a conflict in changelog

Add support for EKS Pod Identity
fix issue: thanos-io#7157

Signed-off-by: farhad <[email protected]>
Updated changelog

Signed-off-by: farhad <[email protected]>
@eqfarhad
Copy link
Contributor Author

eqfarhad commented May 6, 2024

There is a conflict in changelog

I just rebased my branch so it should be fine now

@yeya24 yeya24 enabled auto-merge (squash) May 6, 2024 21:03
@eqfarhad eqfarhad requested a review from yeya24 May 6, 2024 22:17
@yeya24 yeya24 merged commit ab8f2b3 into thanos-io:main May 6, 2024
20 checks passed
Nashluffy pushed a commit to Nashluffy/thanos that referenced this pull request May 14, 2024
* Update minio-go to v7.0.70

Add support for EKS Pod Identity
fix issue: thanos-io#7157

Signed-off-by: farhad <[email protected]>

* Changelog - support for EKS Pod Identity

Updated changelog

Signed-off-by: farhad <[email protected]>

---------

Signed-off-by: farhad <[email protected]>
Signed-off-by: mluffman <[email protected]>
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Just noticed that the changelog entry seems wrong. It shouldn't be under Fixed. Should be Added instead as it is a feature/enhancement.

@eqfarhad
Copy link
Contributor Author

Just noticed that the changelog entry seems wrong. It shouldn't be under Fixed. Should be Added instead as it is a feature/enhancement.

I've opened a new PR to fix the changelog position;

hczhu-db pushed a commit to databricks/thanos that referenced this pull request Aug 22, 2024
* Update minio-go to v7.0.70

Add support for EKS Pod Identity
fix issue: thanos-io#7157

Signed-off-by: farhad <[email protected]>

* Changelog - support for EKS Pod Identity

Updated changelog

Signed-off-by: farhad <[email protected]>

---------

Signed-off-by: farhad <[email protected]>
hczhu-db pushed a commit to databricks/thanos that referenced this pull request Aug 22, 2024
* Update minio-go to v7.0.70

Add support for EKS Pod Identity
fix issue: thanos-io#7157

Signed-off-by: farhad <[email protected]>

* Changelog - support for EKS Pod Identity

Updated changelog

Signed-off-by: farhad <[email protected]>

---------

Signed-off-by: farhad <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants