aws-lib version upgrade + pagination support#296
Conversation
pagination support
|
This pull request does not have a backport label. Could you fix it @oren-zohar? 🙏
|
ofiriro3
left a comment
There was a problem hiding this comment.
Huge work + a lot of refactoring 💯
I found a small breaking change that is critical and we have to address it, the rest is some small comments.
But I'd like to go over it from the beginning just to make sure I hadn't miss anything
| /// If we will ever wish to change it, DescribeRepositories returns results in paginated manner | ||
| func (provider *ECRProvider) DescribeAllECRRepositories(ctx context.Context) ([]ecr.Repository, error) { | ||
| /// When repoNames is nil, it will describe all the existing repositories | ||
| // If we will ever wish to change it, DescribeRepositories returns results in paginated manner |
There was a problem hiding this comment.
No need for this comment anymore
| } | ||
|
|
||
| return response.Repositories, err | ||
| return repos, nil |
There was a problem hiding this comment.
It's good for a first iteration solution, but in the long term, I think that we might want to consider doing it on the infrastructure level.
| github.com/Microsoft/go-winio => github.com/bi-zone/go-winio v0.4.15 | ||
| github.com/Shopify/sarama => github.com/elastic/sarama v1.19.1-0.20210823122811-11c3ef800752 | ||
| github.com/apoydence/eachers => github.com/poy/eachers v0.0.0-20181020210610-23942921fe77 //indirect, see https://github.com/elastic/beats/pull/29780 for details. | ||
| github.com/cucumber/godog => github.com/cucumber/godog v0.8.1 |
There was a problem hiding this comment.
Was it removed on purpose?
There was a problem hiding this comment.
yes, we don't use it
| } | ||
|
|
||
| type EcrRepository ecr.Repository | ||
| type EcrRepository types.Repository |
There was a problem hiding this comment.
To completely finish the amazing segregation that you made, can we replace the types. Repository with a type of our own?
Same as you did in the IAM and the EKS fetchers
That way, only the AWS provider will be aware of the AWS interface
| } | ||
|
|
||
| type LoadBalancersDescription elasticloadbalancing.LoadBalancerDescription | ||
| type LoadBalancersDescription types.LoadBalancerDescription |
There was a problem hiding this comment.
Can we replace types.LoadBalancerDescription with awslib.***?
Same way as you did for EKS fetcher and the IAM fetcher?
| ) | ||
|
|
||
| type EksClusterOutput eks.DescribeClusterOutput | ||
| type EksCluster types.Cluster |
There was a problem hiding this comment.
I think that we don't use the field anymore, are we?
ofiriro3
left a comment
There was a problem hiding this comment.
No further review is necessary! Amazing job
But please take a look at my last two comments 🥇
| @@ -26,6 +26,7 @@ import ( | |||
| ) | |||
|
|
|||
| type ElbLoadBalancerDescriptions []types.LoadBalancerDescription | |||
There was a problem hiding this comment.
nit: isn't the ElbLoadBalancerDescriptions an array of ElbLoadBalancersDescription?
Do we need them both?
| "github.com/aws/aws-sdk-go-v2/service/ecr/types" | ||
| ) | ||
|
|
||
| type EcrRepository types.Repository |
There was a problem hiding this comment.
nit: isn't the EcrRepositories an array of EcrRepository?
Do we need them both?
|
This pull request is now in conflicts. Could you fix it? 🙏 |
Cloudbeat CI 🤖Allure Report: http://csp-allure-reports.s3.amazonaws.com/allure_reports/cloudbeat/prs/296/index.html |
# Conflicts: # resources/fetchers/ecr_factory_test.go # resources/fetchers/ecr_fetcher.go # resources/fetchers/ecr_fetcher_test.go # resources/providers/awslib/ecr_provider.go # resources/providers/awslib/ecr_provider_mock.go # resources/providers/awslib/ecr_public_provider.go
# Conflicts: # go.mod # go.sum
In this PR:
aws-sdk-go-v2version to 1.16.7DescribeRepositoriesandDescribeLoadBalancer