Migrate to openapi-generator and okhttp3

[fabiokung]

Replaces #595. Requires yue9944882/gen#1 and sundrio/sundrio#156, kubernetes/pom.xml needs to be updated in this PR after it gets merged.

Fixes #482.

@yue9944882 @brendanburns I got everything compiling with these changes. They will break backwards compatibility, so a new major version release will be required.

EDIT: more requirements: OpenAPITools/openapi-generator#4226, OpenAPITools/openapi-generator#4182

[k8s-ci-robot]

Welcome @fabiokung!

It looks like this is your first PR to kubernetes-client/java 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/java has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fabiokung
To complete the pull request process, please assign yue9944882
You can assign the PR to them by writing /assign @yue9944882 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[fabiokung]

/assign @yue9944882

[fabiokung]

Tests are also passing with a patched sundrio. The only way I can think of making CI happy before sundrio/sundrio#156 gets merged and released, would be to vendor a patched jar in here (ugh).

[fabiokung]

I take it back, bootstrapping SSL needs some work (okhttp3 does not like a null SSLContextFactory), and the YAML test needs some fixing since the schema changed for IntOrString.

[brendandburns]

What do you mean "Schema changed for IntOrString" the Schema should definitely not change in the API.

The last time I tried this, the custom type registration stuff in the generator (e.g. the stuff that makes IntOrString work correctly) was broken, that was the main problem that prevented us from switching.

Please make sure that IntOrString, Quantity, and the other custom types are parsing correctly.

Thanks!

[fabiokung]

I see. I meant that I was just assuming the new generator explicitly wanted to map IntOrString to String.

I missed the custom generators stuff. Your comment makes sense. I will take a look later today or tomorrow.

[brendandburns]

I had a thread (a while ago) with one of the OpenAPI maintainers. If you send mail to bburns [at] microsoft I will try to add you to the thread where I discussed some of the challenges I saw...

[yue9944882]

hi @fabiokung any plans to revive this thread?

[fabiokung]

Hi! Yes. I just had no time in the past weeks, but I can spend some cycles early next week, unless you want to pick it up?

[yue9944882]

appreciated if you can continue the work! i added this to my list, and will be coming back for review soon!

[yue9944882]

[ERROR] Failed to execute goal on project client-java-api: Could not resolve dependencies for project io.kubernetes:client-java-api:bundle:7.0.0-SNAPSHOT: Failure to find io.sundr:builder-annotations:jar:0.19-SNAPSHOT in https://oss.sonatype.org/content/repositories/snapshots/ was cached in the local repository, resolution will not be reattempted until the update interval of sonatype-snapshots has elapsed or updates are forced -> [Help 1]

CI's complaining about failing to find io.sundr:builder-annotations:jar:0.19-SNAPSHOT, guess we should probably drop the suffix?

[yue9944882]

@fabiokung the diff is so large that my browser can hardly load it, i did review locally in my IDE:

1. pls generate the project from the latest release-1.15 branch, we are not skipping 1.15.

2. plz add the following content back to the .openapi-generator-ignore file

# Remove this once swagger-codegen 2.3.0 is released and we update. 
# Verify the following PRs are in the release:
#   * https://github.com/swagger-api/swagger-codegen/pull/6230
src/main/java/io/kubernetes/client/JSON.java
# src/main/java/io/kubernetes/client/ApiClient.java (NOTE: probably want to add this back for further manual edits)

3. plz resolve the git conflicts

4. i see unexpected diffs in util/src/main/java/io/kubernetes/client/informer/SharedInformerFactory.java, split this out as another PR:

    if (apiClient.getHttpClient().readTimeoutMillis() > 0) {
      log.warn("HttpClient has a read timeout, removing it so watch calls don't terminate early");
      OkHttpClient httpClient =
          apiClient.getHttpClient().newBuilder().readTimeout(0, TimeUnit.MILLISECONDS).build();
      apiClient.setHttpClient(httpClient);
    }

5. in util/src/main/java/io/kubernetes/client/util/WebSocketStreamHandler.java line 109, why is the try-catch block dropped?

--- Thanks, Min

[yue9944882]

@brendandburns do you think we should cut an alpha release for this pull? it will be breaking compatiblities and we can probably missing sth in this migration.. i can start by trying the alpha release out in our internal projects.

[fabiokung]

CI's complaining about failing to find io.sundr:builder-annotations:jar:0.19-SNAPSHOT, guess we should probably drop the suffix?

I mentioned this in the PR description, we need to wait until sundrio/sundrio#156 gets merged, then update this to the right released version. In order to test this I had a locally built snapshot in my local maven repo, with the patched sundrio.

[fabiokung]

@yue9944882 comments inline:

1. pls generate the project from the latest release-1.15 branch, we are not skipping 1.15.

Sure, let me give it a try.

2. plz add the following content back to the .openapi-generator-ignore file

# Remove this once swagger-codegen 2.3.0 is released and we update. 
# Verify the following PRs are in the release:
#   * https://github.com/swagger-api/swagger-codegen/pull/6230
src/main/java/io/kubernetes/client/JSON.java
# src/main/java/io/kubernetes/client/ApiClient.java (NOTE: probably want to add this back for further manual edits)

I'm reviewing the custom types with the new openapi-generator, and we may not need this anymore with openapi-generator.

1. plz resolve the git conflicts
2. i see unexpected diffs in util/src/main/java/io/kubernetes/client/informer/SharedInformerFactory.java, split this out as another PR:

These are not unrelated changes, they are part of moving to okhttp3 (used by openapi-generator). The previous swagger-codegen used an older version of okhttp.

1. in util/src/main/java/io/kubernetes/client/util/WebSocketStreamHandler.java line 109, why is the try-catch block dropped?

Same thing, the close() method with okhttp3 does not throw an IOException anymore.

[fabiokung]

Regenerated against 1.15, and fixed the merge conflict.

Now we wait for OpenAPITools/openapi-generator#4182 to get IntOrString and Quantity working properly.

[fabiokung]

Custom types look OK now. The last remaining issue is that SSL bootstraping in ApiClient (as generated by openapi-generator) is currently throwing NullPointerException.

[brendandburns]

Thanks for the continued effort here, I'm looking forward to merging this!

[fabiokung]

@yue9944882 AFAICT all the necessary changes have been merged to openapi-generator, and sundrio. I will leave it up to you if pointing kubernetes-client/gen to the master branch of openapi-generator is acceptable and regenerating all clients and models with it, or if we need to wait for a formal release of openapi-generator first.

[brendanburns]

Master branch is fine with me. Thanks for the effort here!

…

On Thu, Oct 24, 2019, 4:42 PM Fabio Kung ***@***.***> wrote: @yue9944882 <https://github.com/yue9944882> AFAICT all the necessary changes have been merged to openapi-generator, and sundrio. I will leave it up to you if pointing kubernetes-client/gen to the master branch of openapi-generator is acceptable and regenerating all clients and models with it, or if we need to wait for a formal release of openapi-generator first. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#709?email_source=notifications&email_token=AAJZQXULEF22UCMOM4NP6W3QQIXEXA5CNFSM4IY4SK42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECGX3MA#issuecomment-546143664>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJZQXWN74WZKFKFVNRHK3TQQIXEXANCNFSM4IY4SK4Q> .

[yue9944882]

had confirmed w/ @wing328, the next release 4.2.0 will be landing by the end of october. https://github.com/OpenAPITools/openapi-generator/milestone/25. how about wait for 5-ish days? @brendandburns @fabiokung

[brendanburns]

Waiting 5 days is fine with me. Thanks for the update.

…

On Thu, Oct 24, 2019, 8:46 PM Min Kim ***@***.***> wrote: had confirmed w/ @wing328 <https://github.com/wing328>, the next release 4.2.0 will be landing by the end of october. https://github.com/OpenAPITools/openapi-generator/milestone/25. how about wait for 5-ish days? @brendandburns <https://github.com/brendandburns> @fabiokung <https://github.com/fabiokung> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#709?email_source=notifications&email_token=AAJZQXQRLL4LZ3VMKSTZG4TQQJTZRA5CNFSM4IY4SK42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECHC5WA#issuecomment-546189016>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJZQXXA37FLD4354Y5LEPDQQJTZRANCNFSM4IY4SK4Q> .

[fabiokung]

how about wait for 5-ish days?

sgtm, no worries

[yue9944882]

@fabiokung can you rename the generated package name to io.kubernetes.openapi in settings file per #744？ the old project structure is causing issues in OSGi integration. besides i think we should reserve the old sources and deprecate them by following https://stackoverflow.com/a/23019294.

/**
 * @deprecated As of release 7.0.0, replaced by {@link io.kubernetes.openapi;}
 */
@Deprecated
package io.kubernetes.client; // TODO: completely remove it after release 9.0.0

[fabiokung]

I will give that a try, not sure some of the non-generated classes will be backwards compatible, so keeping old classes may not do much. I will update with more info as I learn more.

[yue9944882]

not sure some of the non-generated classes will be backwards compatible,

can you elaborate? there's no non-generated classes under client-java-api module AFAIK. i agree it won't save us from breaking compatibility but at least comfort those users who find their project broken from compiling after upgrading to new libraries in some sense..

[yue9944882]

@fabiokung am changing my mind, let's completely remove the old generated classes and re-generated w/ the new name io.kubernetes.openapi. it's okay to lost compatibility between major versions.

[fabiokung]

Awesome! I've been pretty swamped this week, but will get to it asap.

[yue9944882]

@fabiokung anything i can help to move it forward? thanks

[fabiokung]

On my end I still haven't been able to carve up some time to work on it. I expect to be able to get it done by the end of this week. Would that work for you? If not, we can find ways to hand over the remaining work to you.

[yue9944882]

@fabiokung @brendandburns i just opened a pull to continue the work from this pull #778, PTAL

[fabiokung]

@yue9944882 would you rather I bring your commits to this PR and rebase everything here, or continue on your PR? There's some important history here, but either way works for me.

[yue9944882]

it's fine to separate the discussion/history and the actual pull #778 i suppose. thank you for your great effort in moving this forward!

[fabiokung]

continuing in #778