Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore(deps): update dependencies #500

Merged
merged 14 commits into from
Sep 16, 2021

Conversation

metacosm
Copy link
Contributor

@metacosm metacosm commented Sep 8, 2021

  • Use Quarkus BOM instead of universe as it's not needed
  • Remove use of CRD Generator from annotation processing as the SDK
    extension takes care of it now. Generated CRD is in a different spot
    now as it's generated when the controllers are processed so they are
    now found under operator/target/kubernetes
  • Had to change the super class of ResourceEvent because AbstractEvent
    got renamed to DefaultEvent but will re-introduce AbstractEvent in a
    new SDK release to revert backwards compatibility that shouldn't have
    been broken

- Use Quarkus BOM instead of universe as it's not needed
- Remove use of CRD Generator from annotation processing as the SDK
  extension takes care of it now. Generated CRD is in a different spot
  now as it's generated when the controllers are processed so they are
  now found under operator/target/kubernetes
- Had to change the super class of ResourceEvent because AbstractEvent
  got renamed to DefaultEvent but will re-introduce AbstractEvent in a
  new SDK release to revert backwards compatibility that shouldn't have
  been broken
Note that this is only needed if we provide a QuarkusApplication,
otherwise the SDK will automatically do it.
@github-actions github-actions bot added api changes related to api operator changes related to operator labels Sep 9, 2021
@shawkins shawkins force-pushed the update-dependencies-new branch from df96614 to e2f43b3 Compare September 9, 2021 00:24
@shawkins shawkins force-pushed the update-dependencies-new branch from e2f43b3 to 3939d0c Compare September 9, 2021 01:42
@github-actions github-actions bot added the test Test related changes label Sep 9, 2021
@shawkins
Copy link
Contributor

shawkins commented Sep 9, 2021

The last issue seems to be with the quarkus generated Service in the sync kubernetes.yml. Instead of using port 8080, it's specifying port 80 - there's no change in our config nor mention of a property to control this. In any case when the Service is using port 80 the external LoadBalancer Service created to access the sync won't actually respond to requests. I've verified that if I manually update the kubernetes.yml to still use port 8080 it works as expected. Need to track down why this change was made in quarkus - asked as https://stackoverflow.com/questions/69122602/change-in-default-service-host-port

@github-actions github-actions bot added the sync changes related to sync label Sep 9, 2021
@shawkins shawkins requested review from ppatierno and k-wall September 9, 2021 17:43
@shawkins
Copy link
Contributor

shawkins commented Sep 9, 2021

Once we are comfortable with these changes, I have another commit ready to remove some of the tech debt.

@metacosm
Copy link
Contributor Author

Tracked the port change down: quarkusio/quarkus#15407

CRD listing permission should be removed from CSV as well.
@k-wall
Copy link
Contributor

k-wall commented Sep 10, 2021

I'm see a local build error when compiling:

java --version
Picked up _JAVA_OPTIONS: -Dvertx.disableDnsResolver=true
openjdk 16.0.2 2021-07-20
OpenJDK Runtime Environment Homebrew (build 16.0.2+0)
OpenJDK 64-Bit Server VM Homebrew (build 16.0.2+0, mixed mode, sharing)**
mvn clean verify
(snip)
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project kas-fleetshard-api: Compilation failure: Compilation failure:
[ERROR] /Users/kwall/src/kas-fleetshard/api/src/main/java/org/bf2/operator/resources/v1alpha1/ManagedKafka.java:[110,34] incompatible types: java.lang.String cannot be converted to char[]
[ERROR] /Users/kwall/src/kas-fleetshard/api/src/main/java/org/bf2/operator/resources/v1alpha1/ManagedKafka.java:[121,45] incompatible types: java.lang.String cannot be converted to char[]
[ERROR] /Users/kwall/src/kas-fleetshard/api/src/main/java/org/bf2/operator/resources/v1alpha1/ManagedKafka.java:[141,60] incompatible types: java.lang.String cannot be converted to char[]

Anyone know what might cause this?

Making changes like this resolves the compile error:

             tls = new TlsKeyPairBuilder()
-                    .withNewCert(endpointTlsCert)
-                    .withNewKey(endpointTlsKey)
+                    .withCert(endpointTlsCert)
+                    .withKey(endpointTlsKey)

@shawkins
Copy link
Contributor

@k-wall you shouldn't be seeing errors. Seems like you are getting a different generator version.

That said there's a lot of stuff in our builders that could be cleaned up. It looks like newX was used quite a bit with strings and the transitive building should work as expected with custom types.

@k-wall
Copy link
Contributor

k-wall commented Sep 10, 2021

@k-wall you shouldn't be seeing errors. Seems like you are getting a different generator version.

That said there's a lot of stuff in our builders that could be cleaned up. It looks like newX was used quite a bit with strings and the transitive building should work as expected with custom types.

This is a Java version issue. If I use Java 11 (like CI), the problem goes away, if I use Java 14 or 16 (my default), I see the compilation error.

@k-wall
Copy link
Contributor

k-wall commented Sep 10, 2021

Tested end to end with kas-installer: apps are functional, logs clean and metrics look reasonable.
Nice work @metacosm @shawkins

@k-wall k-wall added this to the 0.12.0 milestone Sep 10, 2021
@metacosm
Copy link
Contributor Author

Note that we haven't tested with Java 16 so good to know there might be issues with it. Java 17 support is on the roadmap once it's released.

@shawkins
Copy link
Contributor

It looks like newX was used quite a bit with strings and the transitive building should work as expected with custom types.

To fix the transitive stuff we need sundrio .5 or later - sundrio/sundrio#260 However that does not look to be working with this quarkus version, so I won't pursue that yet.

@k-wall
Copy link
Contributor

k-wall commented Sep 12, 2021

It looks like newX was used quite a bit with strings and the transitive building should work as expected with custom types.

To fix the transitive stuff we need sundrio .5 or later - sundrio/sundrio#260 However that does not look to be working with this quarkus version, so I won't pursue that yet.

Agreed. Let's merge this PR.

@shawkins
Copy link
Contributor

Agreed. Let's merge this PR.

Any additional changes will come as a separate pr. It will look something like https://github.com/shawkins/mk-agent/commit/7c706f05c68527fb7be414fb7597479df8dfe9bb

@ppatierno
Copy link
Contributor

So I tested these changes and found an NPE on the fleetshard operator that seems to be related to ConfigProperties and related things get deprecated in new Quarkus version. More details from @shawkins @MikeEdgar who are working on it.
I also tried the process around the Strimzi bundle installation/upgrade due to change related to use the builtin PackageManifest client and all seems to work fine.

@github-actions github-actions bot added the perf changes related to perf label Sep 15, 2021
Signed-off-by: Michael Edgar <medgar@redhat.com>
Copy link
Contributor

@rareddy rareddy left a comment

Choose a reason for hiding this comment

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

lgtm

@shawkins shawkins merged commit dcf2487 into bf2fc6cc711aee1a0c2a:main Sep 16, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
api changes related to api operator changes related to operator perf changes related to perf sync changes related to sync test Test related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants