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

feat(dax): Add support for Dax #1256

Merged
merged 9 commits into from
May 8, 2022
Merged

feat(dax): Add support for Dax #1256

merged 9 commits into from
May 8, 2022

Conversation

DanielTemesgen
Copy link
Contributor

@DanielTemesgen DanielTemesgen commented Apr 11, 2022 •
edited

Description of your changes

Fixes #1195

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

  • Creating a Cluster, ParameterGroup and SubnetGroup
  • Deleting a Cluster, ParameterGroup and SubnetGroup
  • Unit testing

@DanielTemesgen DanielTemesgen marked this pull request as ready for review April 11, 2022 17:45
@DanielTemesgen DanielTemesgen force-pushed the dax-cluster-support branch 3 times, most recently from 0a2f102 to 8f1fbf4 Compare April 11, 2022 20:54
@DanielTemesgen DanielTemesgen changed the title Add support for Dax feat(dax): Add support for Dax Apr 12, 2022
haarchri
haarchri reviewed Apr 18, 2022
View reviewed changes
Copy link
Member

@haarchri haarchri left a comment

Choose a reason for hiding this comment

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

nice first contribution - i get the DAX stuff working in our environment - please have a look for a few nitpicks - after that i will do final review ;)

apis/dax/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
apis/dax/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
apis/dax/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
apis/dax/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
apis/dax/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
pkg/controller/dax/cluster/setup.go Outdated Show resolved Hide resolved
pkg/controller/dax/parametergroup/setup.go Outdated Show resolved Hide resolved
pkg/controller/dax/parametergroup/setup.go Outdated Show resolved Hide resolved
pkg/controller/dax/subnetgroup/setup.go Outdated Show resolved Hide resolved
pkg/controller/dax/subnetgroup/setup.go Outdated Show resolved Hide resolved
@DanielTemesgen
Copy link
Contributor Author

nice first contribution - i get the DAX stuff working in our environment - please have a look for a few nitpicks - after that i will do final review ;)

Thank you for having a look :) Have worked on the suggestions and have a couple of queries

@haarchri
Copy link
Member

i will start testing and check

haarchri
haarchri approved these changes May 8, 2022
View reviewed changes
Copy link
Member

@haarchri haarchri left a comment

Choose a reason for hiding this comment

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

LGTM - found a few small issues with RoleARN extractor and examples - i will push an commit and we are ready ;)

dtemesgen and others added 9 commits May 8, 2022 08:49
@haarchri
Add dax cluster support, unit tests pass
fd5004b 
Signed-off-by: dtemesgen <dtemesgen@expediagroup.com>
@haarchri
Fix linting
936d003 
Signed-off-by: dtemesgen <dtemesgen@expediagroup.com>
@haarchri
Generate 404 errors correctly, fix isUpToDate function in ParameterGr…
065a432 
…oup to cover comparison of parameters

Signed-off-by: dtemesgen <dtemesgen@expediagroup.com>
@haarchri
Ensure that Custom Parameters are properly defined as "required", usi…
1b9b720 
…ng the model json as a guide https://github.com/aws/aws-sdk-go/blob/8d549237533fa5f4ac6d4b0251e37e4facabdd2b/models/apis/dax/2017-04-19/api-2.json.

Signed-off-by: dtemesgen <dtemesgen@expediagroup.com>
@haarchri
Add examples, fix required kubebuilder annotation, fix case where clu…
832c690 
…ster is unavailable while in a "modifying" state

Signed-off-by: dtemesgen <dtemesgen@expediagroup.com>
@haarchri
Rerun build
33c20a9 
Signed-off-by: dtemesgen <dtemesgen@expediagroup.com>
@haarchri
Change yaml example to use references. Set external name of resources…
9dd63db 
… in preCreate, remove postCreate. Remove required annotations.

Signed-off-by: dtemesgen <dtemesgen@expediagroup.com>
@haarchri
Simplify if blocks within Cluster preCreate function
e685f42 
Signed-off-by: dtemesgen <dtemesgen@expediagroup.com>
@haarchri
fix(examples/extractor): added roleARN extractor for cluster and fits…
381f81b 
… examples for dependend resources

Signed-off-by: haarchri <chhaar30@googlemail.com>
@haarchri
Copy link
Member

haarchri commented May 8, 2022

create & delete is working

NAME                                                     READY   SYNCED   EXTERNAL-NAME
subnetgroup.dax.aws.crossplane.io/example-subnet-group   True    True     example-subnet-group

NAME                                                READY   SYNCED   EXTERNAL-NAME
cluster.dax.aws.crossplane.io/example-dax-cluster   True    True     example-dax-cluster

NAME                                                                     READY   SYNCED   EXTERNAL-NAME
parametergroup.dax.aws.crossplane.io/example-parameter-group             True    True     example-parameter-group
parametergroup.dax.aws.crossplane.io/example-parameter-group-no-values   True    True     example-parameter-group-no-values

@haarchri
Copy link
Member

haarchri commented May 8, 2022

rebase master and added commit

@haarchri haarchri merged commit de41888 into crossplane-contrib:master May 8, 2022
@DanielTemesgen
Copy link
Contributor Author

thank you very much for reviewing 😀

febarbosa182 pushed a commit to febarbosa182/provider-aws that referenced this pull request May 23, 2022
@DanielTemesgen @haarchri @febarbosa182
feat(dax): Add support for Dax (crossplane-contrib#1256)
2fa6851 
* Add dax cluster,subnetgroup and parametergroup support

Signed-off-by: dtemesgen <dtemesgen@expediagroup.com>
Co-authored-by: haarchri <chhaar30@googlemail.com>
Signed-off-by: Felipe Barbosa <lybrbarbosa@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haarchri haarchri haarchri approved these changes

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

Successfully merging this pull request may close these issues.

DAX Cluster Support
2 participants
@DanielTemesgen @haarchri