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 #3973: removing the need for parameter processing in Serialization #4651

Closed
wants to merge 3 commits into from

Conversation

shawkins
Copy link
Contributor

@shawkins shawkins commented Dec 8, 2022

Description

Moves #3972 further along by deprecating Serialization methods accepting parameters. Also support for Parameterizable has been removed - it should have been rarely used. The code to handle template parameters in now localized to processLocally.

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

…ialization

The methods accepting parameters have been deprecated
Support for Parameterizable has been removed - it should have been
rarely used.
also moving more of the openshift impl to be client based
@shawkins
Copy link
Contributor Author

shawkins commented Dec 9, 2022

@manusa @rohanKanojia sorry I expanded this pr. The changes are:

  1. Deprecate Serialization taking responsibility for parameter replacement.
  2. Consolidate all of the template parameter replacement logic to the TemplateOperationsImpl - it had been in three places. To do this we are getting rid of the Parameterizable interface. It was only needed in scenarios where there was a non-string parameter, which we now handle. We also had overloaded the load logic in several places related to templates - however that causes a duplication in logic and makes the behavior harder to document / understand. To replace the behavior of templates().load, there is now a templates().from method - which is appropriately documented. I understand that all of this would be a breaking change, but since it only affects templates it may be acceptable for 6.x. If not we can hold off until 7 and put deprecations in now.
  3. With a lot of the logic cleared out, I started to make more progress on refining away the need for openshift-client jar. I can't continue on that path until Client.resource(resource).get() does not return latest resource #4574 is included as more conflicts will emerge.

Should I continue with this pr with the serialization refinements, or just wait for #4574 and this are merged before picking that up again?

@sonarcloud
Copy link

sonarcloud bot commented Dec 11, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 43 Code Smells

26.5% 26.5% Coverage
0.0% 0.0% Duplication

@shawkins
Copy link
Contributor Author

@manusa @metacosm After talking over the in the meeting, I'll close this pr and split the other as:

  1. In an initial pr deprecating parameter logic - Parameterizable, and on Serialization. Also add a warning that behavior of load with Templates will change - it will no longer implicitly process the templates. Ideally deprecate the access / clearing of the jsonMapper and yamlMappers - but I won't have anything to replace that, simply a statement that the methods will be going away.
  2. After the GET refinement is committed, a follow on pr for 7.0 containing the work that pull out the parameter handling.
  3. After that pr, I can refine the logic to remove the need for the OpenShift client jar.
  4. After that pr, I can tackle removing the static deserialization.

Once I have things in better shape, I'll also close the other draft pr until we're more ready for the actual changes.

@shawkins
Copy link
Contributor Author

Closing in favor of starting with #4683

Will refine the second pr out of what's here / what's on #4662

@shawkins shawkins closed this Dec 14, 2022
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

1 participant