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

feat(kafka): Add update function for cluster #1714

Merged

Conversation

akesser
Copy link
Contributor

@akesser akesser commented Mar 31, 2023

Description of your changes

We implemented the functions to update a Kafka cluster

fixes the update of a Kafka cluster as mentioned in #1683

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

Tested locally against aws account

pkg/controller/kafka/cluster/setup.go Outdated Show resolved Hide resolved
pkg/controller/kafka/cluster/setup.go Outdated Show resolved Hide resolved
@akesser akesser requested a review from MisterMX April 6, 2023 09:37
@haarchri
Copy link
Member

haarchri commented Apr 6, 2023

@akesser can you Check for DCO ?

Signed-off-by: André Kesser <andre.kesser@dkb.de>
@akesser akesser force-pushed the feature/kafka-cluster-update branch from 84c8547 to 63d8cdd Compare April 11, 2023 10:00
@haarchri haarchri closed this Apr 11, 2023
@haarchri haarchri reopened this Apr 11, 2023
Copy link
Collaborator

@MisterMX MisterMX left a comment

Choose a reason for hiding this comment

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

@akesser could you unwrap the ifs into a more linear form. Otherwise the code is too hard to read. Also you might want to checkout cmp.Equal to see if can help you to save some of that code.

pkg/controller/kafka/cluster/setup.go Show resolved Hide resolved
pkg/controller/kafka/cluster/setup.go Show resolved Hide resolved
pkg/controller/kafka/cluster/setup.go Show resolved Hide resolved
pkg/controller/kafka/cluster/setup.go Show resolved Hide resolved
pkg/controller/kafka/cluster/setup.go Show resolved Hide resolved
pkg/controller/kafka/cluster/setup.go Show resolved Hide resolved
pkg/controller/kafka/cluster/setup.go Show resolved Hide resolved
pkg/controller/kafka/cluster/setup.go Show resolved Hide resolved
@akesser
Copy link
Contributor Author

akesser commented May 30, 2023

According to its self description, cmp.Equal is not meant to be used in production:

This package is intended to be a more powerful and safer alternative to reflect.DeepEqual for comparing whether two values are semantically equal. It is intended to only be used in tests, as performance is not a goal and it may panic if it cannot compare the values. Its propensity towards panicking means that it's unsuitable for production environments where a spurious panic may be fatal. (from: https://pkg.go.dev/github.com/google/go-cmp/cmp#pkg-overview)

In my opinion, the used ifs is the best way to keep the code readable, keeping it clear what property is under test right now. Using linearized forms of ifs requires many repetitions, making it hard to see the difference in the if to the previous and following test.

The code in other files also uses nested ifs, not to mention auto generated code.

@MisterMX
Copy link
Collaborator

MisterMX commented Jun 1, 2023

@akesser cmp.Equal is used extensively in this provider and in many others as well. The performance hit, if noticable at all, is negliable because the API calls take much more time. Since we know all of the types that we want to compare it is safe to assume that they will be comparable.

What counts is source code readability and multiple nested ifs aren't exactly that. It might be that other controllers do that but that doesn't make it any more readable.

It would be really great if you could simplify the code by using it.

@akesser
Copy link
Contributor Author

akesser commented Jun 5, 2023

Additionally to the warning, that cmp.Equal should not be used, the different naming of some properties, like IAM vs Iam or EBSStorageInfo vs. EbsStorageInfo would lead to either ignoring them in cmp.Equal and comparing them by hand again or writing transformers that do the same comparison the code already does.

@MisterMX
Copy link
Collaborator

MisterMX commented Jun 5, 2023

That's correct, the common way is to create a *svcapitypes.LoggingInfo from the *svcsdk.LoggingInfo so compare two objects of the same type.

@haarchri
Copy link
Member

think the code is Not easier to read when switching to cmp.Equal

@MisterMX MisterMX changed the title feat: add update function for kafka cluster feat(kafka): Add update function for cluster Jun 16, 2023
Copy link
Collaborator

@MisterMX MisterMX left a comment

Choose a reason for hiding this comment

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

Alright, then its decided. Thank you for your contribution @akesser!

@MisterMX MisterMX merged commit 811c2c0 into crossplane-contrib:master Jun 16, 2023
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.

None yet

3 participants