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

(MINOR) improve R53 AssertHostedZoneExists func #20

Merged
merged 3 commits into from
Dec 21, 2021
Merged

Conversation

mdb
Copy link
Contributor

@mdb mdb commented Dec 21, 2021

SUMMARY

DEVELOPER IMPACT

While this does change the method name from AssertRoute53HostedZoneExists to AssertHostedZoneExists -- and therefore, in theory, introduces a breaking change -- I don't believe the original AssertRoute53HostedZoneExists was functioning properly due to issue #19

Contribution Checklist

  • All changes have been reflected in comparable unit test changes or additions.
  • Any interactions with third party clients are done via interface types rather than direct interactions.
  • All new functions follow the required naming standard.
  • All new functions follow the required signature standards.

Discussion question

This PR changes the func name from AssertRoute53HostedZoneExists to AssertHostedZoneExists, as per PR 15 discussion:

I think this would be better named as AssertRecordExistsInHostedZone, since route53 is already part of the package name.

However, I believe ☝️ is not quite correct. route53 is not actually part of the package name, if I understand correctly. The package name is aws. So, should the func names include *Route53* or not? As a frame of reference, the EC2-related funcs do appear to include *EC2* in their names: https://github.com/HBOCodeLabs/infratest/blob/main/pkg/aws/ec2.go#L57

Update: I've changed both Route53 assertion functions to include *Route53* in their name, as is consistent with the other infratest functions.

mdb added 2 commits December 21, 2021 10:04
This makes the function naming more consistent with that of `AssertRecordExistsInHostedZone` as
was originally suggested here:
#15 (comment)

> I think this would be better named as AssertRecordExistsInHostedZone, since route53 is already part of the package name.
@mdb mdb requested a review from a team as a code owner December 21, 2021 15:23
yardbirdsax
yardbirdsax previously approved these changes Dec 21, 2021
Copy link
Member

@yardbirdsax yardbirdsax left a comment

Choose a reason for hiding this comment

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

Approved. Agreed that this doesn't need to be a major version bump, especially since we are still in 0.x versions so there shouldn't be a hard expectation of interface stability yet.

@yardbirdsax
Copy link
Member

However, I believe ☝️ is not quite correct. route53 is not actually part of the package name, if I understand correctly. The package name is aws. So, should the func names include Route53 or not?

Hmm... you appear to be correct, perhaps I was not fully ☕ 'd up that day. Given the pattern established with the EC2 methods, I agree that the methods here should likely include Route53 in their name.

Per discussion, it seems the inclusion of `*Route53*` is perhaps best for consistency's sake:
#20 (comment)
@mdb
Copy link
Contributor Author

mdb commented Dec 21, 2021

@yardbirdsax Thanks for the feedback. I've pushed another commit such that both Route53 assertion functions include Route53 in their names.

Copy link
Member

@yardbirdsax yardbirdsax left a comment

Choose a reason for hiding this comment

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

LGTM. Please just call out the changes in a ## CHANGES section in the extended description of your merge commit, and include (MINOR) in your main title. You can check the last commit for an example. Thanks for the work!

@mdb mdb changed the title improve R53 AssertHostedZoneExists func (MINOR) improve R53 AssertHostedZoneExists func Dec 21, 2021
@mdb mdb merged commit 632881c into main Dec 21, 2021
@mdb mdb deleted the mdb/improve-r53-func branch December 21, 2021 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants