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

Fix Redis provisioning; Improve Redis with the latest AliCloud SDK and support for Cluster instances #113

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

Melonbun233
Copy link

Description of your changes

This PR fixed the deprecated AliCloud Redis provisioning.
It also provides various new features supported from AliCloud Redis including:

  • Fixed duplicated provisioning due to old version of crossplane runtime
  • Support of appending whitelist IPs after instance provisioning
  • Improve initial user creation with default instance id
  • Support of modifying instance spec in update
  • Support of modifying instance config in update
  • Support of Cluster, stand-alone, and read-split Redis instance provisioning
  • 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

Unit tests are fixed with the latest change.
Manual integration tests to use the controller to provision a Redis instance with different parameters and updating specs are executed.

upgrade golang to 1.20

Signed-off-by: Henry Zeng <[email protected]>
remove recommend in error due to requestID

add more comment

Signed-off-by: Henry Zeng <[email protected]>
update parameters for redis spec

generate new crds for redis

fix typo

Signed-off-by: Henry Zeng <[email protected]>
update parameters to provision redis

Signed-off-by: Henry Zeng <[email protected]>
remove unneeded connection sync

update generated redis crd

Signed-off-by: Henry Zeng <[email protected]>
Signed-off-by: Henry Zeng <[email protected]>
Signed-off-by: Henry Zeng <[email protected]>
Signed-off-by: Henry Zeng <[email protected]>
Signed-off-by: Henry Zeng <[email protected]>
Signed-off-by: Henry Zeng <[email protected]>
Signed-off-by: Henry Zeng <[email protected]>
Copy link
Member

@jbw976 jbw976 left a comment

Choose a reason for hiding this comment

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

@Melonbun233 and @dee0sap brought up this PR at the latest Crossplane community meeting on June 27 2024. There has not been a release in almost 2 years for this provider, and progress is not moving forward for this PR. The current maintainers have been challenging to get a hold of. That can happen in the diverse range of extensions in the Crossplane ecosystem and we'll need to define a good process for it 🙇‍♂️

Agenda doc/notes: https://docs.google.com/document/d/1q_sp2jLQsDEOX7Yug6TPOv7Fwrys6EwcF5Itxjkno7Y/edit#bookmark=id.akrfonjz569m

Timestamp link to recording: https://youtu.be/0AL1Gl5B3JY?si=gpO4k0UHlTATe8-C&t=2458

Their team is using this provider and would like to see this functionality added so they can keep taking a dependency on it for their use cases going forward. It is possible as well that they may have the resources to step up to maintain and steward the provider as well, so that it will continue to receive fixes in a timely manner for their needs and by extension the greater community's needs.

I have a follow-up to discuss with the steering committee a process that we can follow for these types of scenarios, but I'd like to work right now under the "good faith" assumption that progress on this provider is good and @Melonbun233 @dee0sap's team will be willing to help out in a way that is beneficial for the overall Crossplane ecosystem.

Let's start first with getting this PR to a good spot we feel comfortable with, then we can consider next steps to merge, release, and potentially expand the maintainer team.

@Melonbun233 - would you be able to update the PR description to include more details about your testing? e.g. the manifests you used, the status/results of the resources in AliCloud that show it is healthy and functioning, etc.? Thoroughness on this testing will be very helpful specifically in the context of this PR 😇

Copy link
Member

Choose a reason for hiding this comment

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

i expect this file can be added to gitignore

Copy link

Choose a reason for hiding this comment

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

Thanks, Added the file to gitignore.

---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.3.0
controller-gen.kubebuilder.io/version: v0.8.0
Copy link
Member

Choose a reason for hiding this comment

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

were there any changes in these CRDs that you were worried about for breaking changes?

Copy link

Choose a reason for hiding this comment

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

Only the changes to Redis related CRDs will introduce breaking changes.
I tried with the existing implementation, but the older version of alicloud SDK returns error because multiple parameters have been deprecated.
I had to upgrade SDK version, which deprecated some old parameters and introduced new parameters for Redis provisioning.

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.

3 participants