Skip to content

Comments

AWS: Add TagSession support in AssumeRoleAwsClientFactory#4358

Merged
jackye1995 merged 1 commit intoapache:masterfrom
xiaoxuandev:add-assume-role-tags
Mar 18, 2022
Merged

AWS: Add TagSession support in AssumeRoleAwsClientFactory#4358
jackye1995 merged 1 commit intoapache:masterfrom
xiaoxuandev:add-assume-role-tags

Conversation

@xiaoxuandev
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the AWS label Mar 17, 2022
@xiaoxuandev xiaoxuandev force-pushed the add-assume-role-tags branch from 0bbc125 to 01e36b2 Compare March 18, 2022 00:23
Copy link
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

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

looks good to me, waiting for CI pass

@jackye1995
Copy link
Contributor

jackye1995 commented Mar 18, 2022

return String.format("iceberg-aws-%s", UUID.randomUUID());
}

private static Set<Tag> toTags(Map<String, String> properties) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a similar method in AwsProperties. Can we re-use that?

private Set<Tag> toTags(Map<String, String> properties, String prefix) {
return PropertyUtil.propertiesWithPrefix(properties, prefix)
.entrySet().stream()
.map(e -> Tag.builder().key(e.getKey()).value(e.getValue()).build())
.collect(Collectors.toSet());
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we cannot reuse, this is STS tag, not S3 tag, different class.

Copy link
Contributor

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks @xiaoxuandev for this !!!

@jackye1995 jackye1995 merged commit a86f08c into apache:master Mar 18, 2022
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.

4 participants