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

Update deprecated method in OpenShiftClient #1208

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

jedla97
Copy link
Member

@jedla97 jedla97 commented Jul 18, 2024

Summary

Removing deprecated methods createOrReplace() and yamlMapper() from OpenShiftClient

Tested this version with testsuite weekly stable ocp and test was passing except know issues.

The createOrReplace was replaced by createOr and update is now provided by NonDeletingOperation::update or NonDeletingOperation::patch. This was recomnded by official FAQ.
I decided to use the unlock even as it's optional as per explanation it's needed if the resourceVersion has value.
The use of the unlock function is optional and is only needed if you are starting with a item that has the resourceVersion populated.

For applyServicePropertiesToDeployment method I using patch as I encounter few problem when testing it. Sometime it throw that resources are outdated when applying update. In this case patch is more safe. The problem was only in configMapEndToEnd but it's used in multiple tests. If you want to use update insted of patch we probably need to implement some kind of retry mechanism for it

The yamlMapper was updated similar as they done it when removing they usage here

Please check the relevant options

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Dependency update
  • Refactoring
  • Release (follows conventions described in the RELEASE.md)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Example scenarios has been updated / added
  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@jedla97
Copy link
Member Author

jedla97 commented Jul 18, 2024

run tests

1 similar comment
@mjurc
Copy link
Member

mjurc commented Aug 5, 2024

run tests

@rsvoboda
Copy link
Member

rsvoboda commented Aug 6, 2024

@mjurc do you think the fail in OCP native is related to the change?

@jedla97
Copy link
Member Author

jedla97 commented Aug 6, 2024

@rsvoboda I saw this failiure few times in past. That test is flaky/need more time don't know.

But I tested it with testsuite and saw few failiures, but didn't have time to investegate them. But this PR is still on my radar and will finish it when I'll have some spare hours.

@mjurc
Copy link
Member

mjurc commented Aug 6, 2024

Hey, I just used this PR to check that the credentials in the workflow worked proper - sorry about that

@jedla97 jedla97 force-pushed the remove-deprecated-methods branch 2 times, most recently from 43aaf89 to 36f2900 Compare September 6, 2024 15:36
@jedla97
Copy link
Member Author

jedla97 commented Sep 9, 2024

run tests

1 similar comment
@jedla97
Copy link
Member Author

jedla97 commented Sep 9, 2024

run tests

@jedla97 jedla97 marked this pull request as ready for review September 9, 2024 18:43
@jedla97
Copy link
Member Author

jedla97 commented Sep 9, 2024

The Linux JVM is not caused by this, same as daily
Will wait for ocp tests

@jedla97
Copy link
Member Author

jedla97 commented Sep 10, 2024

run tests

@jedla97 jedla97 requested a review from mjurc September 10, 2024 11:19
// call createOrReplace
client.resource(groupModel).createOrReplace();
// call createOr and if it exists the update will be done
client.resource(groupModel).unlock().createOr(NonDeletingOperation::update);
Copy link
Member

Choose a reason for hiding this comment

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

Gee, I am surprised there's actually an elegant solution like this. Thank you!

@mjurc
Copy link
Member

mjurc commented Sep 10, 2024

Let's try it :)

@mjurc mjurc merged commit 6338a6f into quarkus-qe:main Sep 10, 2024
9 of 10 checks passed
@jedla97 jedla97 added the triage/backport-1.5? Quarkus 3.15 stream label Sep 10, 2024
@jedla97 jedla97 deleted the remove-deprecated-methods branch September 10, 2024 12:29
@michalvavrik michalvavrik removed the triage/backport-1.5? Quarkus 3.15 stream label Sep 13, 2024
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.

4 participants