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(redis): add Redis Cluster support #623

Merged
merged 3 commits into from
Oct 25, 2024
Merged

feat(redis): add Redis Cluster support #623

merged 3 commits into from
Oct 25, 2024

Conversation

rickard-von-essen
Copy link
Contributor

Description of your changes

This adds support for google_redis_cluster Terraform resource, see Terraform - Google - redis_cluster.

Replaces #513

I have:

  • Read and followed Crossplane's [contribution process].
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary. N/A

How has this code been tested

I have tested, create, import, update, and delete of a Memorystore Redis Cluster. And also verified that the AdditionalConnectionDetailsFn functions.

$ kubectl get clusters.redis.gcp.upbound.io
NAME              SYNCED   READY   EXTERNAL-NAME     AGE
crossplane-test   True     True    crossplane-test   9m45s

One odd thing is that examples-generated/redis/v1beta1/cluster.yaml contains:

 zoneDistributionConfig:
  - mode: MULTI_ZONE

But it has to be changed to:

  zoneDistributionConfig:
    mode: MULTI_ZONE

Otherwise the apply fails.

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/redis/v1beta1/cluster.yaml"

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/redis/v1beta1/cluster.yaml"

@rickard-von-essen rickard-von-essen force-pushed the feature/redis-cluster branch 3 times, most recently from a8884e1 to 9ebbb41 Compare September 26, 2024 12:31
@rickard-von-essen
Copy link
Contributor Author

@jeanduplessis Could I get a final (hopefully) /test-example... on this 🙏🏻

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/redis/v1beta1/cluster.yaml"

@rickard-von-essen
Copy link
Contributor Author

Fails because of missing prerequisite Service Connection Policy. Which I started on in #624 but Upjet fails to correctly generate a References 😢

 logger.go:42: 07:44:25 | case/0-apply |       message: 'async create failed: failed to create the resource: [***0 Error waiting
    logger.go:42: 07:44:25 | case/0-apply |         to create Cluster: Error waiting for Creating Cluster: Error code 9, message:
    logger.go:42: 07:44:25 | case/0-apply |         Invalid resource state for "projects/official-provider-testing/locations/us-central1/clusters/cluster-ha":
    logger.go:42: 07:44:25 | case/0-apply |         No service connection policy is associated with project: official-provider-testing,
    logger.go:42: 07:44:25 | case/0-apply |         network: projects/official-provider-testing/global/networks/producer-net,
    logger.go:42: 07:44:25 | case/0-apply |         region: us-central1. Create a service connection policy following https://cloud.google.com/vpc/docs/configure-service-connection-policies

@rickard-von-essen
Copy link
Contributor Author

Depends on #624

@negz negz mentioned this pull request Oct 16, 2024
Open
@rickard-von-essen
feat(redis): add Redis Cluster support
793ee09 
This adds support for google_redis_cluster Terraform resource, see
https://registry.terraform.io/providers/hashicorp/google/5.39.1/docs/resources/redis_cluster

Signed-off-by: Rickard von Essen <rickard.von.essen@gmail.com>
@turkenf
Copy link
Collaborator

turkenf commented Oct 23, 2024

@rickard-von-essen, now that you are a contributor, you can trigger uptests.

@rickard-von-essen
Copy link
Contributor Author

Awesome! All work on getting this sorted tomorrow.

@rickard-von-essen
Copy link
Contributor Author

/test-examples="examples/redis/v1beta1/cluster.yaml"

@rickard-von-essen
Copy link
Contributor Author

Uptest-examples/redis/v1beta1/cluster.yaml — Passed

I think this is ready for review now that all tests passes.

turkenf
turkenf reviewed Oct 24, 2024
View reviewed changes
Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

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

Thank you @rickard-von-essen, I left two comments about AdditionalConnectionDetailsFn for you to consider.

config/redis/config.go Outdated Show resolved Hide resolved
config/redis/config.go Outdated Show resolved Hide resolved
@rickard-von-essen
Put all discovery endpoints in AdditionalConnectionDetails
9cc2c51 
Signed-off-by: Rickard von Essen <rickard.von.essen@gmail.com>
@rickard-von-essen
Copy link
Contributor Author

/test-examples="examples/redis/v1beta1/cluster.yaml"

1 similar comment
@turkenf
Copy link
Collaborator

turkenf commented Oct 25, 2024

/test-examples="examples/redis/v1beta1/cluster.yaml"

@rickard-von-essen
Increase timeout to 30 min
e8f9838 
Signed-off-by: Rickard von Essen <rickard.von.essen@gmail.com>
@turkenf
Copy link
Collaborator

turkenf commented Oct 25, 2024

/test-examples="examples/redis/v1beta1/cluster.yaml"

turkenf
turkenf approved these changes Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

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

Thank you @rickard-von-essen, LGTM 🙌

@turkenf turkenf merged commit cefe60d into crossplane-contrib:main Oct 25, 2024
10 checks passed
@rickard-von-essen rickard-von-essen deleted the feature/redis-cluster branch October 26, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turkenf turkenf turkenf approved these changes

@ulucinar ulucinar Awaiting requested review from ulucinar ulucinar is a code owner

@sergenyalcin sergenyalcin Awaiting requested review from sergenyalcin sergenyalcin is a code owner

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

Successfully merging this pull request may close these issues.
3 participants
@rickard-von-essen @jeanduplessis @turkenf