Conversation
Signed-off-by: Andrej Podhradsky <apodhrad@redhat.com>
| self.ssm_connection = boto3client('ssm', **connection_kwargs) | ||
| self.sns_connection = boto3client('sns', **connection_kwargs) | ||
| self.cw_events_connection = boto3client('events', **connection_kwargs) | ||
| self.resourcegroupstaggingapi_connection = boto3client('resourcegroupstaggingapi', **connection_kwargs) |
There was a problem hiding this comment.
That's a heck of an endpoint name 😆
There was a problem hiding this comment.
btw not at all suggesting you change it - its consistent, and is exactly what it should be. It's just ... verbose.
| "region": self._region_name, | ||
| "identifier": [ | ||
| { | ||
| "kubernetes.io/cluster/" + ocp_name: "owned" |
There was a problem hiding this comment.
Please use f-strings for concatenation
| if ocp_item["name"] == ocp_name: | ||
| ocp = ocp_item | ||
| break | ||
| if not ocp: |
There was a problem hiding this comment.
Consider using for/else syntax here, removes the need to initalize ocp and you're already using a break.
| ocp_list = [] | ||
| resources = self.resourcegroupstaggingapi_connection.get_resources()["ResourceTagMappingList"] | ||
| for resource in resources: | ||
| tags = resource["Tags"] |
There was a problem hiding this comment.
No need to label tags here if its only used in the for statement, just reference resource['Tags'] directly
|
|
||
| def list_ocps(self): | ||
| """ | ||
| List openshift clusters (name, metadata, resources) where metadata can be used for uninstalling the cluster |
There was a problem hiding this comment.
Why limit the return to clusters with this metadata?
| tags = resource["Tags"] | ||
| for tag in tags: | ||
| key = tag["Key"] | ||
| if key.startswith("kubernetes.io/cluster/"): |
There was a problem hiding this comment.
The GetResources endpoint supports TagFilters, and you can use a key regex expression to limit the response to only resources with this tag. Then you don't have to iterate so much here, and can directly fetch the objects that you want and parse their cluster key.
https://docs.aws.amazon.com/resourcegroupstagging/latest/APIReference/API_GetResources.html
https://docs.aws.amazon.com/resourcegroupstagging/latest/APIReference/API_TagFilter.html
mshriver
left a comment
mshriver
left a comment
There was a problem hiding this comment.
Couple things to discuss, the function could use some more explicit docs to describe the intended filtering/exclusion/inclusion that is happening, and why. Consider broader uses than your intent for cloudwash cleanup, and maybe move the metadata/tags restriction to a kwarg so the caller can more broadly list OCP clusters.
Thanks so much for adding to the functionality of wrapanapi!
|
Thanks for the review! I will apply your suggestions everywhere it is possible. |
|
@apodhrad Ping ! Do you need any help with this PR ? |
|
@apodhrad Ping again! |
|
to me it seems that this PR is outdated in favor of |
|
I think this PR will be replaced by another (better) PR. @oharan2 could you provide more details if we can close this PR, please? |
This PR can be closed and replaced by me with new changes rebased on top of @apodhrad work. Note: This PR sets the approach of using the |
5 participants
Signed-off-by: Andrej Podhradsky apodhrad@redhat.com