Add bom module by honnix · Pull Request #1402 · operator-framework/java-operator-sdk

honnix · 2022-08-16T10:26:56Z

When a project has multiple modules, publishing a bom is considered to be a good practice for users to more easily manage dependencies. For example: https://search.maven.org/artifact/io.fabric8/kubernetes-client-bom and https://search.maven.org/artifact/io.micrometer/micrometer-bom.

I'm not sure of the actual artifact deployment process, so hopefully this makes sense.

I tested locally and it works.

metacosm

LGTM. Thank you!

honnix · 2022-08-16T14:22:25Z

Not full sure I understand the error. It seems the build is checking code format, including pom.xml, right? But the pom.xml for bom does not have the plugin declared.

honnix · 2022-08-16T14:51:15Z

I tried fixing the issue by skipping the bom module -pl '!operator-framework-bom' for some of the maven runs.

metacosm · 2022-08-18T08:25:42Z

Upon further review, I'm wondering if the BOM should be part of the project itself (i.e. use the parent) or stays standalone as is currently done… Any informed opinion on the subject?

honnix · 2022-08-18T08:29:48Z

Upon further review, I'm wondering if the BOM should be part of the project itself (i.e. use the parent) or stays standalone as is currently done… Any informed opinion on the subject?

@metacosm Thanks for replying. That's a good question. It is considered to be a good practice not using project parent in BOM, to avoid leaking dependencies.

metacosm · 2022-08-18T08:37:17Z

Upon further review, I'm wondering if the BOM should be part of the project itself (i.e. use the parent) or stays standalone as is currently done… Any informed opinion on the subject?

@metacosm Thanks for replying. That's a good question. It is considered to be a good practice not using project parent in BOM, to avoid leaking dependencies.

That's kind of what I was thinking but couldn't find a definitive answer on the topic after an admittedly rather short online search… Do you have any reference on the subject?

honnix · 2022-08-18T09:48:05Z

Upon further review, I'm wondering if the BOM should be part of the project itself (i.e. use the parent) or stays standalone as is currently done… Any informed opinion on the subject?

@metacosm Thanks for replying. That's a good question. It is considered to be a good practice not using project parent in BOM, to avoid leaking dependencies.

That's kind of what I was thinking but couldn't find a definitive answer on the topic after an admittedly rather short online search… Do you have any reference on the subject?

I can try to find something. We had a few leakages internally that caused trouble, but I unfortunately cannot reveal much.

The problem is, if bom has parent pointing to the project root, all the managed deps of the project root will be imported to user project if user imports the bom.

metacosm · 2022-08-18T09:54:44Z

The problem is, if bom has parent pointing to the project root, all the managed deps of the project root will be imported to user project if user imports the bom.

Yes so let's go with the current organisation, just made a naming change.

honnix · 2022-08-18T10:10:19Z

Just for the record, I created a project with minimum deps.

  <dependencyManagement>
    <dependencies>
      <dependency>
        <groupId>io.javaoperatorsdk</groupId>
        <artifactId>operator-framework-bom</artifactId>
        <version>3.1.2-SNAPSHOT</version>
        <type>pom</type>
        <scope>import</scope>
      </dependency>
    </dependencies>
  </dependencyManagement>

  <dependencies>
    <dependency>
      <groupId>io.javaoperatorsdk</groupId>
      <artifactId>operator-framework-core</artifactId>
    </dependency>
    <dependency>
      <groupId>io.javaoperatorsdk</groupId>
      <artifactId>micrometer-support</artifactId>
    </dependency>
    <dependency>
      <groupId>org.apache.commons</groupId>
      <artifactId>commons-lang3</artifactId>
      <version>3.11.0</version>
    </dependency>
  </dependencies>

Note that when not defining parent for operator-framework-bom, I will have to give commons-lang3 a version otherwise maven fails.

$ mvn dependency:tree

[INFO] com.example:foo:jar:0.0.1-SNAPSHOT
[INFO] +- io.javaoperatorsdk:operator-framework-core:jar:3.1.2-SNAPSHOT:compile
[INFO] |  +- io.fabric8:kubernetes-client:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-core:jar:5.12.3:compile
[INFO] |  |  |  +- io.fabric8:kubernetes-model-common:jar:5.12.3:compile
[INFO] |  |  |  \- com.fasterxml.jackson.core:jackson-annotations:jar:2.13.1:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-rbac:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-admissionregistration:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-apps:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-autoscaling:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-apiextensions:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-batch:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-certificates:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-coordination:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-discovery:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-events:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-extensions:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-flowcontrol:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-networking:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-metrics:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-policy:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-scheduling:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-storageclass:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-node:jar:5.12.3:compile
[INFO] |  |  +- com.squareup.okhttp3:okhttp:jar:3.12.12:compile
[INFO] |  |  |  \- com.squareup.okio:okio:jar:1.15.0:compile
[INFO] |  |  +- com.squareup.okhttp3:logging-interceptor:jar:3.12.12:compile
[INFO] |  |  +- com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:jar:2.13.1:compile
[INFO] |  |  |  \- org.yaml:snakeyaml:jar:1.28:compile
[INFO] |  |  +- com.fasterxml.jackson.datatype:jackson-datatype-jsr310:jar:2.13.1:compile
[INFO] |  |  +- com.fasterxml.jackson.core:jackson-databind:jar:2.13.1:compile
[INFO] |  |  +- com.fasterxml.jackson.core:jackson-core:jar:2.13.1:compile
[INFO] |  |  +- io.fabric8:zjsonpatch:jar:0.3.0:compile
[INFO] |  |  \- com.github.mifmif:generex:jar:1.0.2:compile
[INFO] |  |     \- dk.brics.automaton:automaton:jar:1.11-8:compile
[INFO] |  \- org.slf4j:slf4j-api:jar:1.7.36:compile
[INFO] +- io.javaoperatorsdk:micrometer-support:jar:3.1.2-SNAPSHOT:compile
[INFO] |  \- io.micrometer:micrometer-core:jar:1.9.3:compile
[INFO] |     +- org.hdrhistogram:HdrHistogram:jar:2.1.12:compile
[INFO] |     \- org.latencyutils:LatencyUtils:jar:2.0.3:runtime
[INFO] \- org.apache.commons:commons-lang3:jar:3.11.0:compile        <--------------

If I define parent for operator-framework-bom, I can simply do:

  <dependencyManagement>
    <dependencies>
      <dependency>
        <groupId>io.javaoperatorsdk</groupId>
        <artifactId>operator-framework-bom</artifactId>
        <version>3.1.2-SNAPSHOT</version>
        <type>pom</type>
        <scope>import</scope>
      </dependency>
    </dependencies>
  </dependencyManagement>

  <dependencies>
    <dependency>
      <groupId>io.javaoperatorsdk</groupId>
      <artifactId>operator-framework-core</artifactId>
    </dependency>
    <dependency>
      <groupId>io.javaoperatorsdk</groupId>
      <artifactId>micrometer-support</artifactId>
    </dependency>
    <dependency>
      <groupId>org.apache.commons</groupId>
      <artifactId>commons-lang3</artifactId>  <!-- version leaked from parent pom -->
    </dependency>
  </dependencies>

And I get:

$ mvn dependency:tree

[INFO] com.example:foo:jar:0.0.1-SNAPSHOT
[INFO] +- io.javaoperatorsdk:operator-framework-core:jar:3.1.2-SNAPSHOT:compile
[INFO] |  +- io.fabric8:kubernetes-client:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-core:jar:5.12.3:compile
[INFO] |  |  |  +- io.fabric8:kubernetes-model-common:jar:5.12.3:compile
[INFO] |  |  |  \- com.fasterxml.jackson.core:jackson-annotations:jar:2.13.1:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-rbac:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-admissionregistration:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-apps:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-autoscaling:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-apiextensions:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-batch:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-certificates:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-coordination:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-discovery:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-events:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-extensions:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-flowcontrol:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-networking:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-metrics:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-policy:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-scheduling:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-storageclass:jar:5.12.3:compile
[INFO] |  |  +- io.fabric8:kubernetes-model-node:jar:5.12.3:compile
[INFO] |  |  +- com.squareup.okhttp3:okhttp:jar:4.10.0:compile
[INFO] |  |  |  +- com.squareup.okio:okio-jvm:jar:3.0.0:compile
[INFO] |  |  |  |  \- org.jetbrains.kotlin:kotlin-stdlib-common:jar:1.5.31:compile
[INFO] |  |  |  \- org.jetbrains.kotlin:kotlin-stdlib:jar:1.6.20:compile
[INFO] |  |  |     \- org.jetbrains:annotations:jar:13.0:compile
[INFO] |  |  +- com.squareup.okhttp3:logging-interceptor:jar:4.10.0:compile
[INFO] |  |  |  \- org.jetbrains.kotlin:kotlin-stdlib-jdk8:jar:1.6.10:compile
[INFO] |  |  |     \- org.jetbrains.kotlin:kotlin-stdlib-jdk7:jar:1.6.10:compile
[INFO] |  |  +- com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:jar:2.13.1:compile
[INFO] |  |  |  \- org.yaml:snakeyaml:jar:1.28:compile
[INFO] |  |  +- com.fasterxml.jackson.datatype:jackson-datatype-jsr310:jar:2.13.1:compile
[INFO] |  |  +- com.fasterxml.jackson.core:jackson-databind:jar:2.13.1:compile
[INFO] |  |  +- com.fasterxml.jackson.core:jackson-core:jar:2.13.1:compile
[INFO] |  |  +- io.fabric8:zjsonpatch:jar:0.3.0:compile
[INFO] |  |  \- com.github.mifmif:generex:jar:1.0.2:compile
[INFO] |  |     \- dk.brics.automaton:automaton:jar:1.11-8:compile
[INFO] |  \- org.slf4j:slf4j-api:jar:1.7.36:compile
[INFO] +- io.javaoperatorsdk:micrometer-support:jar:3.1.2-SNAPSHOT:compile
[INFO] |  \- io.micrometer:micrometer-core:jar:1.9.3:compile
[INFO] |     +- org.hdrhistogram:HdrHistogram:jar:2.1.12:compile
[INFO] |     \- org.latencyutils:LatencyUtils:jar:2.0.3:runtime
[INFO] \- org.apache.commons:commons-lang3:jar:3.12.0:compile        <--------------

So there org.apache.commons:commons-lang3:jar:3.12.0 is leaked from parent pom.

metacosm · 2022-08-18T10:35:39Z

Yes, we should try to remove that dependency altogether…

honnix · 2022-08-18T11:38:04Z

Yes, we should try to remove that dependency altogether…

I just randomly picked up a dependency from parent, and this leakage applies to any managed dependency in parent.

Please feel free to merge this whenever suitable. Thank you so much for reviewing and providing great feedback.

metacosm · 2022-08-18T14:10:17Z

Yes, we should try to remove that dependency altogether…

I just randomly picked up a dependency from parent, and this leakage applies to any managed dependency in parent.

Yes, I understood that point but this particular dependency is something we've been considering getting rid of for a while, actually :)

Please feel free to merge this whenever suitable. Thank you so much for reviewing and providing great feedback.

Thank you for your contribution to this project!

honnix · 2022-08-18T14:13:03Z

Yes, I understood that point but this particular dependency is something we've been considering getting rid of for a while, actually :)

Nice!. Fewer is better. :D

csviri

LGTM

csviri · 2022-08-22T09:37:28Z

+        </dependencies>
+    </dependencyManagement>
+
+    <distributionManagement>


Just to my understanding, why distribution management is here?

And why just snapshot?

Good question. Since we cannot inherit from parent for bom as explained in previous comments, and parent pom has https://github.com/java-operator-sdk/java-operator-sdk/blob/main/pom.xml#L190-L195, so I just replicated here.

Hmm, I guess this should be defined in the end project where the framework is used. Not here. But not sure tbh.

https://central.sonatype.org/publish/publish-maven/#other-prerequisites seems to explain why snapshotRepository is needed when using nexus-staging-maven-plugin (this project uses it in release profile).

csviri

We should probably use this in the examples.

honnix · 2022-08-22T10:53:35Z

We should probably use this in the examples.

@csviri Do you mean doing that in this PR? That might require slightly bigger changes since the sample module inherits those dependencies from parent, so if we start importing from bom, it's better to cut that off (the same dep leakage issue as described above).

honnix · 2022-08-22T10:55:32Z

Hmm, it's actually discarding dependencyManagement https://github.com/java-operator-sdk/java-operator-sdk/blob/main/sample-operators/tomcat-operator/pom.xml#L28

csviri · 2022-08-22T11:53:51Z

We should probably use this in the examples.

@csviri Do you mean doing that in this PR? That might require slightly bigger changes since the sample module inherits those dependencies from parent, so if we start importing from bom, it's better to cut that off (the same dep leakage issue as described above).

Not necessarily in this PR. But in that case would be better to target the next branch with it. (Just rebase it onto next and edit the PR to target next.) Since it's not a patch, rather a new feature for 3.2. Also we would like to keep main releasable with patches all the time.

* Add bom module * Skip operator-framework-bom * fix: naming Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>

metacosm approved these changes Aug 16, 2022

View reviewed changes

metacosm self-requested a review August 18, 2022 08:25

metacosm approved these changes Aug 18, 2022

View reviewed changes

Comment thread operator-framework-bom/pom.xml Outdated

metacosm requested a review from csviri August 18, 2022 14:39

csviri approved these changes Aug 22, 2022

View reviewed changes

csviri reviewed Aug 22, 2022

View reviewed changes

csviri requested changes Aug 22, 2022

View reviewed changes

honnix and others added 3 commits August 22, 2022 14:16

Add bom module

c9d1453

Skip operator-framework-bom

babaa6f

fix: naming

f02ed4d

honnix force-pushed the bom branch from 06df073 to f02ed4d Compare August 22, 2022 12:17

honnix changed the base branch from main to next August 22, 2022 12:17

csviri approved these changes Aug 22, 2022

View reviewed changes

csviri merged commit bfdb4b0 into operator-framework:next Aug 22, 2022

csviri pushed a commit that referenced this pull request Aug 23, 2022

feat: add bom module (#1402)

d2f0ec1

* Add bom module * Skip operator-framework-bom * fix: naming Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>

csviri pushed a commit that referenced this pull request Aug 24, 2022

feat: add bom module (#1402)

56c6ef8

* Add bom module * Skip operator-framework-bom * fix: naming Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>

csviri pushed a commit that referenced this pull request Aug 25, 2022

feat: add bom module (#1402)

1218e88

* Add bom module * Skip operator-framework-bom * fix: naming Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>

honnix deleted the bom branch August 29, 2022 13:53

csviri pushed a commit that referenced this pull request Aug 30, 2022

feat: add bom module (#1402)

30df84c

* Add bom module * Skip operator-framework-bom * fix: naming Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>

csviri pushed a commit that referenced this pull request Sep 5, 2022

feat: add bom module (#1402)

588d274

* Add bom module * Skip operator-framework-bom * fix: naming Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>

Conversation

honnix commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metacosm left a comment

Choose a reason for hiding this comment

Uh oh!

honnix commented Aug 16, 2022

Uh oh!

honnix commented Aug 16, 2022

Uh oh!

metacosm commented Aug 18, 2022

Uh oh!

honnix commented Aug 18, 2022

Uh oh!

metacosm commented Aug 18, 2022

Uh oh!

honnix commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

metacosm commented Aug 18, 2022

Uh oh!

honnix commented Aug 18, 2022

Uh oh!

metacosm commented Aug 18, 2022

Uh oh!

honnix commented Aug 18, 2022

Uh oh!

metacosm commented Aug 18, 2022

Uh oh!

honnix commented Aug 18, 2022

Uh oh!

csviri left a comment

Choose a reason for hiding this comment

Uh oh!

csviri Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

csviri Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

honnix Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

csviri Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

honnix Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

csviri left a comment

Choose a reason for hiding this comment

Uh oh!

honnix commented Aug 22, 2022

Uh oh!

honnix commented Aug 22, 2022

Uh oh!

csviri commented Aug 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

honnix commented Aug 16, 2022 •

edited

Loading

honnix commented Aug 18, 2022 •

edited

Loading