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.

Sign up for GitHub

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

Jump to bottom

Add support for ResourceRecordSets #358

Merged
merged 9 commits into from
Aug 26, 2021
Merged

Add support for ResourceRecordSets #358

merged 9 commits into from
Aug 26, 2021

Conversation

Feggah
Copy link
Collaborator

@Feggah Feggah commented Aug 24, 2021

Description of your changes

Fixes #350

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

  • Manually tested creating, updating and deleting RRS in GCP.
  • Developed unit tests

@Feggah @cnatan
Add ResourceRecordSet to apis folder
c72de92 
Signed-off-by: Feggah <gabidferreira9@gmail.com>
@Feggah @cnatan
Generate code and manifests
964595c 
Signed-off-by: Feggah <gabidferreira9@gmail.com>
@Feggah @cnatan
Add YAML example
da242ab 
Signed-off-by: Feggah <gabidferreira9@gmail.com>
@Feggah @cnatan
Upgrade SDK version
77a4caa 
Upgrade needed to use CRUD methods from the SDK

Signed-off-by: Feggah <gabidferreira9@gmail.com>
@Feggah @cnatan
Add ResourceRecordSet controller and client implementation
739cff4 
Signed-off-by: Feggah <gabidferreira9@gmail.com>
@Feggah Feggah mentioned this pull request Aug 24, 2021
Open
@Feggah
Bump go version to 1.16 in CI
d55b659 
Signed-off-by: Feggah <gabidferreira9@gmail.com>
turkenh
turkenh reviewed Aug 24, 2021
View reviewed changes
Copy link
Contributor

@turkenh turkenh left a comment

Choose a reason for hiding this comment

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

@Feggah Thanks a lot for working on this, it looks pretty good in general!

I also tested on my side and worked well with create/update/delete. Added a couple of comments which are mostly non-blocking.

pkg/controller/dns/resource_record_set.go Outdated Show resolved Hide resolved
pkg/controller/dns/resource_record_set.go Outdated Show resolved Hide resolved
pkg/controller/dns/resource_record_set.go Outdated Show resolved Hide resolved
apis/dns/v1alpha1/resource_record_set_types.go Outdated Show resolved Hide resolved
examples/dns/resource_record_set.yaml Show resolved Hide resolved
examples/dns/resource_record_set.yaml Outdated Show resolved Hide resolved
pkg/clients/dns/resource_record_set.go Outdated Show resolved Hide resolved
apis/dns/v1alpha1/resource_record_set_types.go Outdated Show resolved Hide resolved
pkg/controller/dns/resource_record_set.go Outdated Show resolved Hide resolved
@Feggah
Remove observation fields; Fix array type
593d5d2 
Signed-off-by: Feggah <gabidferreira9@gmail.com>
@Feggah
Fix controller after changes in types;Add custom initializer
bd3323c 
Signed-off-by: Feggah <gabidferreira9@gmail.com>
@Feggah Feggah requested a review from turkenh August 25, 2021 19:09
turkenh
turkenh reviewed Aug 26, 2021
View reviewed changes
pkg/clients/dns/resource_record_set.go Outdated Show resolved Hide resolved
pkg/clients/dns/resource_record_set.go Outdated Show resolved Hide resolved
pkg/controller/dns/resource_record_set.go Outdated Show resolved Hide resolved
turkenh
turkenh approved these changes Aug 26, 2021
View reviewed changes
Copy link
Contributor

@turkenh turkenh left a comment

Choose a reason for hiding this comment

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

Looking great, just added two more minor comments.

@Feggah Feggah requested a review from turkenh August 26, 2021 12:56
@Feggah
Change initialize and late initialize conditions
28ffcac 
Signed-off-by: Feggah <gabidferreira9@gmail.com>
@Feggah
Copy link
Collaborator Author

Feggah commented Aug 26, 2021

Thanks for your review, @turkenh . Learned a lot with it :)

I think everything is fixed right now.

@turkenh
Copy link
Contributor

turkenh commented Aug 26, 2021

Thanks for your review, @turkenh . Learned a lot with it :)

I think everything is fixed right now.

Thanks a lot for your contribution, great work!

@turkenh turkenh merged commit 77ba296 into crossplane-contrib:master Aug 26, 2021
@Feggah Feggah mentioned this pull request Aug 26, 2021
Closed
@Feggah Feggah mentioned this pull request Oct 14, 2021
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turkenh turkenh Awaiting requested review from turkenh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for ResourceRecordSets
2 participants
@Feggah @turkenh