-
Notifications
You must be signed in to change notification settings - Fork 154
Update maven from 3.5 to 3.6.3 and required dependencies #184
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 maven from 3.5 to 3.6.3 and required dependencies #184
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: abelsromero The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This also includes: * Upgraded openapi-generator-maven-plugin for all generators to 4.3.1. This is the latest v4.x. There's a 5.x but didn't want ot push too much. I'd rather do it in separated PR. * Added skipValidateSpec option to several generations, otherwise the k8s was considered not valid. * Updated installed packaged from Debian 9 to Debian 10. This is the version used by the new maven image. NOTE: using maven plugin for csharp would allow to get rid of this extra packages I suspect. * Updated dotnet installation according to official docs https://docs.microsoft.com/en-us/dotnet/core/install/linux-debian#debian-10- * Some minnor fixes: remove unnecessary maven depencies, format,...
5b666ff
to
9fa5e7e
Compare
To fix csharp code-gen we need to pin at version 2 so we should set see: kubernetes-client/csharp#563 We can do that in this PR or a separate PR |
@@ -8,7 +8,7 @@ | |||
<build> | |||
<plugins> | |||
<plugin> | |||
<groupId>org.openapitools</groupId> | |||
<groupId>org.openapitools</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This indentation is weird, please format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, you need to be consistent with the rest of the file and use spaces.
@@ -21,7 +21,7 @@ ARG OPENAPI_GENERATOR_USER_ORG=OpenAPITools | |||
RUN apt-get update && apt-get -y install git python-pip && pip install urllib3==1.24.2 | |||
|
|||
# Install Autorest | |||
RUN apt-get update && apt-get -qq -y install libunwind8 libicu57 libssl1.0 liblttng-ust0 libcurl3 libuuid1 libkrb5-3 zlib1g |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are required for autorest/dotnet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only changes are:
- Removed libicu57: could not find equivalent for debian10.
- Replaced libcurl3 by libcurl4: which is the "offical" replacement. Though there are some mentions of not all software being compatible.
About if the are required, once we have csharp working again we hopefuly be able to put that at rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a look here:
https://github.com/dotnet/core/blob/main/Documentation/linux-prereqs.md
libicu63 is the debian 10 replacement.
We need to validate that this doesn't introduce any code differences in the generated clients before this can be merged. Ideally this would be a pre-merge check, but sadly it's not currently added as a test. |
I can give it a try in this same PR. |
Running with
|
I'll take a look at getting the csharp generation working. There was a commit to do this here: But it got overwritten for some reason. |
Taking a step back to the beginning, what is the motivation for upgrading to Java8/Maven 3.6? Is there something broken with the existing setup? |
Mainly keeping things with up-to-date and supported versions. |
Ok, given that there's no pressing need, I think I would like to delay this PR until we get csharp generating correctly. I'll try to get to that soon-ish. Apologies for the delay. |
1 similar comment
Ok, given that there's no pressing need, I think I would like to delay this PR until we get csharp generating correctly. I'll try to get to that soon-ish. Apologies for the delay. |
No problem at all. JS it not in my area of expertise, but I can do some more research and share what I find. |
Tried to run autorest "natively" in my machine and it seems to go further, but finally fails with
I have libssl installed, but probably not a compatible version, so I'll try to build a Debian VM. I noted that while running node starts listen on ports 12494 (LISTEN) and 32066 (LISTEN), I wonder if it could be related. |
@abelsromero thanks for pursuing this. Definitely no obligation if you get bored/stuck I will get it done, but the assist is much appreciated! |
Seeeing the interdependencies between tools and how these prevents other converters. I wonder if it would make sense to have different Dockerfiles. I know it would mean some duplication, but the fact that for one language we have to block the mantainance of of al the rest seems a high cost to me. |
@abelsromero probably good observation. We could split them out and have them all depend on a shared base image, which would largely eliminate the duplication. If this is blocking some work on java generation, let me know and I will increase the priority of fixing this. |
@abelsromero: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This PR begun with the intent to update the maven version used 3.5 (2018-06-21) on java8 by latest 3.6.3 (2019-11-25) in java11.
But this broke other things and ended up requiring several changes in all generators.
Now, all but
csharp
work, which fails with the following error. I suspect it could be the ammount of types in the k8s API is too much.Note also that CSharp does not currently work neither because it downloads latest version of
autorest
which requires a newer version of dotnet.We could try to use the openapi maven plugin, based on this example, there's support for csharp https://openapi-generator.tech/docs/usage/, I cannot validate further than checking than some code is being generated. wdyt, should I replace current autorest usage?
Said, that this PR required the following changes:
openapi-generator-maven-plugin
for all generators to 4.3.1. This is the latest v4.x. There's a 5.x but didn't want ot push too much. I'd rather do it in separated PR.skipValidateSpec
option to several generations, otherwise the k8s was considered not valid.