-
Notifications
You must be signed in to change notification settings - Fork 157
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 support for route53 health check #259
Conversation
Codecov Report
@@ Coverage Diff @@
## main #259 +/- ##
==========================================
+ Coverage 69.21% 69.25% +0.04%
==========================================
Files 229 234 +5
Lines 5068 5143 +75
==========================================
+ Hits 3508 3562 +54
- Misses 1279 1295 +16
- Partials 281 286 +5
|
"dynamodb:DescribeContinuousBackups" | ||
"dynamodb:DescribeContinuousBackups", | ||
"route53:ListHealthChecks", | ||
"route53:GetHealthCheck" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we try to keep these permissions in order and put the two new route53 policies just next to the others. Thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll group them by service
go.sum
Outdated
@@ -126,6 +126,7 @@ github.com/aws/aws-sdk-go v1.30.12/go.mod h1:5zCpMtNQVjRREroY7sYe8lOMRSxkhG6MZve | |||
github.com/aws/aws-sdk-go v1.31.9/go.mod h1:5zCpMtNQVjRREroY7sYe8lOMRSxkhG6MZveU8YkpAk0= | |||
github.com/aws/aws-sdk-go v1.34.2 h1:9vCknCdTAmmV4ht7lPuda7aJXzllXwEQyCMZKJHjBrM= | |||
github.com/aws/aws-sdk-go v1.34.2/go.mod h1:5zCpMtNQVjRREroY7sYe8lOMRSxkhG6MZveU8YkpAk0= | |||
github.com/aws/aws-sdk-go v1.37.8 h1:9kywcbuz6vQuTf+FD+U7FshafrHzmqUCjgAEiLuIJ8U= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this mandatory ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm not sure I'll try without that
}, | ||
{ | ||
test: "cannot list health check", | ||
dirName: "route53_health_check_list_error", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep 2 empty results since the first test has the same result ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are testing error behavior there it's not the same think that testing no results are returned from AWS side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the product point of view it is indeed not the same, but as of the mocked golden file reader we return a slice of []resource.Resource
which here is an empty array.
I can easily see a future where we could want to test other errors and where with this logic the number of useless results can only increase exponentially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could test this line like this:
{
test: "cannot list health check",
dirName: "route53_health_check_empty",
mocks: func(client *mocks.Route53Repository) {
client.On("ListAllHealthChecks").Return(nil, awserr.NewRequestFailure(nil, 403, ""))
},
err: remoteerror.NewResourceEnumerationError(awserr.NewRequestFailure(nil, 403, ""), resourceaws.AwsRoute53HealthCheckResourceType),
}
Look at the dirName
here. I reused the one with empty results so that we don't need to create another empty results but with different naming. I did it like that in SQS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I suggest that we stop at the error testing if it fail so we use no folder at all for error testing ? it's not really useful to test the list values and it's not mandatory that this return an empty list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok it cannot work as we need the schema. I think we should find a way to use only one schema if we want to save space I'm not a really big fan of generating golden file for a test and using them for another...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with that, we should find a way to reuse schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok then let's break it into 2 PRs, this one and mine on cloudfront will still have this kind of different folder with its own results and schema. Another PR will deal with a new architecture where we use only one schema per resource. Are you ok with that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for now i reused empty test folder like you asked, But yeah we should do a dedicated pr to refactor this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/remote/aws/test/route53_health_check_multiple/results.golden.json
Outdated
Show resolved
Hide resolved
69eadc0
to
9e32da4
Compare
9e32da4
to
bef20d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/resource/aws/aws_security_group_rule_test.go
bef20d1
to
e191d53
Compare
fakeClient := mocks.Route53Repository{} | ||
c.mocks(&fakeClient) | ||
provider := testmocks.NewMockedGoldenTFProvider(c.dirName, providerLibrary.Provider(terraform.AWS), shouldUpdate) | ||
SNSTopicDeserializer := awsdeserializer.NewRoute53HealthCheckDeserializer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just change here the naming of the variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π€¦
5e14638
to
4b10221
Compare
4b10221
to
f18ce79
Compare
Description
add support for route53 health check