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 #3972: preparing for removal of unneeded serialization logic. #4683

Merged
merged 3 commits into from Dec 22, 2022

Conversation

shawkins
Copy link
Contributor

Description

Furthers #3972 by deprecating (de)serialization logic that we won't want in the future.

Type of change

  • 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

@shawkins
Copy link
Contributor Author

@manusa after reviewing this again I believe we should proceed with a narrow breaking change. Instead of supporting OpenShiftClient.load implicitly processing templates, the callers will need to instead call OpenShiftClient.templates().load - then call an appropriate processing method (get, process, or processLocally). This is semantically much clearer, won't mysteriously change in behavior if you aren't using the openshift client, and eliminates some of the awkward openshift specific logic, which also introduced a duplication of the parameter processing.

I have left for now that the template operations of get and load are overloaded - that is if you have specified parameters, get will interpolate them, and load will interpolate - and convert from the item(s) to a template. That latter behavior seems contrived, does it match some oc functionality?

This latest commit also fixes a problem with the KubernetesClientBuilder - the withConfig methods that deserialize aren't specifying a type, and of course Config is not a KubernetesResource, so that fails.

@manusa
Copy link
Member

manusa commented Dec 15, 2022

OpenShiftClient.templates().load - then call an appropriate processing method (get, process, or processLocally).

Completely agreed. Our DSL should be explicit and provide the semantics that describe what is actually happening which (AFAIK) is a behavior specific to our client.

I have left for now that the template operations of get and load are overloaded - that is if you have specified parameters, get will interpolate them, and load will interpolate - and convert from the item(s) to a template. That latter behavior seems contrived, does it match some oc functionality?

I have no idea. I'm not sure if the parameters were added just as a workaround to be able to work with the templates (using placeholders in field where the type wouldn't support it).

In any case, this will work for a smooth transition to the new methods.

This latest commit also fixes a problem with the KubernetesClientBuilder - the withConfig methods that deserialize aren't specifying a type, and of course Config is not a KubernetesResource, so that fails.

Is this a regression affecting 6.3?

@shawkins
Copy link
Contributor Author

I have no idea. I'm not sure if the parameters were added just as a workaround to be able to work with the templates (using placeholders in field where the type wouldn't support it).

What you can do right now is client.templates().load("anything") - and you'll get back a TemplateResource, that is a single item or items are converted into a Template with a random name with those resources as items. I'm not sure when that is useful.

If you do something like client.templates().withParameters(...)...get|load - those parameters will be applied logically prior to deserialization which was a workaround for the non-string parameter issue. For the purposes of this pr, I'm leaving that functionality in, but deprecating withParameters. After it is removed you will do something like client.templates()...processLocally(parametes) instead.

Is this a regression affecting 6.3?

Not a regression, it's been like that.

all parameter operations are now localized to template logic (overloaded
get and load)

also fixes kubernetesclientbuilder.withconfig
@sonarcloud
Copy link

sonarcloud bot commented Dec 20, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

28.2% 28.2% Coverage
0.0% 0.0% Duplication

@manusa manusa added this to the 6.4.0 milestone Dec 22, 2022
@manusa manusa merged commit 1d3e263 into fabric8io:master Dec 22, 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

2 participants