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

Remove the deprecated Fabric8 method createOrReplace from the project #8310

Merged
merged 7 commits into from Apr 5, 2023
Merged

Remove the deprecated Fabric8 method createOrReplace from the project #8310

merged 7 commits into from Apr 5, 2023

Conversation

im-konge
Copy link
Member

@im-konge im-konge commented Mar 28, 2023 •
edited

Type of change

  • Refactoring

Description

This PR removes the deprecated Fabric8 method createOrReplace.
Instead, we should use the create() or update() methods depending on the use case.
As part of this PR, I'm also adding few methods for creating and updating various resources and changing it in the tests.
Also, some of the resources like ClusterRoles, CRDs, etc., need to have createOrUpdate method for updating the resource when it already exists (because of leftovers from different tests etc.).

Fixes #8270

Checklist

  • Make sure all tests pass

@im-konge im-konge requested review from see-quick, Frawless and a team March 28, 2023 14:56
@im-konge im-konge self-assigned this Mar 28, 2023
@im-konge im-konge added this to the 0.35.0 milestone Mar 28, 2023
@im-konge
Copy link
Member Author

/azp run regression

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

scholzj
scholzj reviewed Mar 28, 2023
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Wuldn't it be begtter to actually know what you are doing and use either create or update? I'm quite sure that not all of these cases have actually a real reason to not use one or another.

@im-konge
Copy link
Member Author

I think there are two options for this:

  1. check the usages as you mention here and use the create/update depending on that
  2. do some encapsulation as you did for secrets (or something like that) -> use create and when an exception about an already existing resource appears, do the update

WDYT?

@scholzj
Copy link
Member

scholzj commented Mar 28, 2023

Well, I think first of all, if you have methods like this: public void create(KafkaConnect resource) => then they should clearly do one of the following:

  • Use create() to create the resource
  • Be renamed to correspond to what they actually do in the code

But I think in most cases you should really know whether you want to create something or update something. Only in cases when you really have good reasons why you don't know whether you are creating or updating something you should use what I did in the UO or use the server apply. I thought I had it in my case in UO. But I'm not sure that is the case here => I guess there might be legit cases for it, but somehow this PR did not convince me that it was really considered.

TBH, if I just wanted to replace the deprecated calls without any further consideration, I would have done it myself and not opened any issues for it.

You should probably also think if server-side apply really does what you want here as it is not the same as replace.

@im-konge
use create or update correctly, instead of createOrReplace and server…
7f05105 
…SideApply

Signed-off-by: Lukas Kral <lukywill16@gmail.com>
@im-konge
fixups
86bffc7 
Signed-off-by: Lukas Kral <lukywill16@gmail.com>
@im-konge
Copy link
Member Author

@scholzj based on your comments, I went through the code and used the create() and update() methods in case where are needed. I will anyway have to run all tests if I didn't break anything else.

I guess this is now correct solution?

@im-konge
Copy link
Member Author

/azp run regression

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

scholzj
scholzj reviewed Mar 29, 2023
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

This looks better code wise, yes. Thanks.

@im-konge
changes and test fixups
87c8f86 
Signed-off-by: Lukas Kral <lukywill16@gmail.com>
@im-konge
Copy link
Member Author

/azp run regression

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
fix for test and javadoc
dc76e88 
Signed-off-by: Lukas Kral <lukywill16@gmail.com>
@im-konge
Copy link
Member Author

im-konge commented Apr 1, 2023

/azp run regression

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

im-konge commented Apr 1, 2023

/azp run upgrade

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

im-konge commented Apr 1, 2023

@scholzj @Frawless @see-quick this is ready for review now.

@im-konge im-konge changed the title Remove the deprecated Fabric8 method createOrUpdate from the project Remove the deprecated Fabric8 method createOrReplace from the project Apr 1, 2023
@im-konge im-konge requested a review from scholzj April 1, 2023 21:48
@im-konge
remove serverSideApply
70af3d7 
Signed-off-by: Lukas Kral <lukywill16@gmail.com>
@im-konge
Copy link
Member Author

im-konge commented Apr 2, 2023

@strimzi-ci run tests test_only profile=all groups=olm

@strimzi-ci
Copy link

▶️ Build started - check Jenkins for more info. ▶️

@strimzi-ci
Copy link

✅ Build SUCCESS - check Jenkins for more info. ✅

@im-konge
check status code in kube exception and add more javadoc
a7e6948 
Signed-off-by: Lukas Kral <lukywill16@gmail.com>
@im-konge im-konge requested a review from scholzj April 3, 2023 12:01
see-quick
see-quick approved these changes Apr 3, 2023
View reviewed changes
Copy link
Member

@see-quick see-quick left a comment

Choose a reason for hiding this comment

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

LGTM, assuming that test will pass

@im-konge
Copy link
Member Author

im-konge commented Apr 3, 2023

/azp run regression

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

scholzj
scholzj approved these changes Apr 3, 2023
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

LGTM assuming the tests pass.

@im-konge
fixes
e0cfd40 
Signed-off-by: Lukas Kral <lukywill16@gmail.com>
@im-konge
Copy link
Member Author

im-konge commented Apr 5, 2023

/azp run regression

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

im-konge commented Apr 5, 2023

/azp run upgrade

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge im-konge mentioned this pull request Apr 5, 2023
Merged
1 task
@im-konge im-konge merged commit 8bdd7e8 into strimzi:main Apr 5, 2023
23 checks passed
@im-konge im-konge deleted the remove-fabric8-deprication branch April 5, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scholzj scholzj scholzj approved these changes

@Frawless Frawless Frawless approved these changes

@see-quick see-quick see-quick approved these changes

Assignees

@im-konge im-konge

Labels
needs review
Projects
None yet
Milestone
0.35.0
Development

Successfully merging this pull request may close these issues.

[ST] Replace createOrReplace() calls in the systemtests module
5 participants
@im-konge @scholzj @strimzi-ci @Frawless @see-quick