improve: blocklist of problematic resources for previous version annotation #2774

csviri · 2025-04-28T12:12:28Z

Signed-off-by: Attila Mészáros [email protected]

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

csviri · 2025-04-29T11:50:28Z

related issues:
#2509
#2553

xstefank · 2025-04-29T13:00:12Z

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

+   *
+   * @return blocklist of resource classes where the previous version annotation won't be used.
+   */
+  default List<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist() {


I would suggest also documenting the default implementation in the javadoc.

Not sure that is needed / that helpful since users can just open the code and see the defaults, also we don't do it for other configs.

What would make more sense, though, is to explain what is the consequence for a resource type to be in the block list because this isn't clear at all and people might be hesitant adding a resource to the block list without more details of what happens when they do.

Also, this should probably be a set rather than a list as the order is meaningless whereas we only want one instance of a given resource type in this collection.

Explained the consequence and changed it to set, thank you!

metacosm · 2025-04-30T14:51:00Z

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

+   *
+   * @return blocklist of resource classes where the previous version annotation won't be used.
+   */
+  default List<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist() {


What would make more sense, though, is to explain what is the consequence for a resource type to be in the block list because this isn't clear at all and people might be hesitant adding a resource to the block list without more details of what happens when they do.

metacosm · 2025-04-30T14:52:16Z

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

+   * @return blocklist of resource classes where the previous version annotation won't be used.
+   */
+  default List<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist() {
+    return List.of(Deployment.class, StatefulSet.class);


Another aspect is whether or not these 2 classes should always be added to the block list regardless of what the user might set this list to (i.e. what is returned by this method should be added to the default list, not replace it completely).

Those should not be always added, if the user is sure that the matcher is right those can be removed.

Not sure I understand. From what I understand, Deployment and StatefulSet are always problematic for the matcher, no?

No, only if it uses fields that are normalized later by the API, (and maybe if some defaults filled in, not sure about that)

In the light of this, it is debatable if those resource should be opt-in or opt-out. WDYT?

metacosm · 2025-04-30T14:53:40Z

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

+   *
+   * @return blocklist of resource classes where the previous version annotation won't be used.
+   */
+  default List<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist() {


Also, this should probably be a set rather than a list as the order is meaningless whereas we only want one instance of a given resource type in this collection.

metacosm · 2025-04-30T14:54:19Z

...core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java

@@ -188,6 +190,12 @@ public ConfigurationServiceOverrider withCloneSecondaryResourcesWhenGettingFromC
    return this;
  }

+  public ConfigurationServiceOverrider previousAnnotationUsageBlocklist(
+      List<Class<? extends HasMetadata>> previousAnnotationUsageBlacklist) {
+    this.previousAnnotationUsageBlocklist = previousAnnotationUsageBlacklist;


See the previous comment on how this should probably be adding to the default set rather than completely replace it.

Responded above.

metacosm · 2025-04-30T14:56:57Z

...k/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/DeploymentDependent.java

+        .build();
+  }
+
+  // for testing purposes replicating the matching logic but with the special matcher


This should use the configuration option from #2760 instead.

If that is merged, can change it (might be a new PR). thx!

xstefank · 2025-05-12T06:43:36Z

...ava/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockReconcilerIT.java

+      LocallyRunOperatorExtension.builder()
+          // Removing resource from blocklist List would result in test failure
+          //          .withConfigurationService(
+          //              o -> o.previousAnnotationUsageBlocklist(Collections.emptyList()))


Should this be a new test then?

It could be an additional test, I did not add that since I did not find it important to show that something is not working.

csviri · 2025-05-13T11:46:08Z

@metacosm are there any other concerns or we can merge this?

Signed-off-by: Attila Mészáros <[email protected]>

Signed-off-by: Chris Laprun <[email protected]>

csviri · 2025-05-16T11:36:31Z

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

@@ -474,7 +474,8 @@ default boolean previousAnnotationForDependentResourcesEventFiltering() {
   *
   * @return a Set of resource classes where the previous version annotation won't be used.
   */
-  default Set<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist() {
+  default Set<Class<? extends HasMetadata>>
+      previousAnnotationForDependentResourcesEventFilteringBlocklist() {


I think this is fine to have also with shorter form and keep it consistent. (withPreviousAnnotationForDependentResourcesBlocklist)

Signed-off-by: Attila Mészáros <[email protected]>

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2025

csviri requested review from shawkins, metacosm and xstefank April 28, 2025 12:15

csviri changed the title ~~feat: blacklist of problematic resources for previous version annotation~~ improve: blacklist of problematic resources for previous version annotation Apr 28, 2025

csviri commented Apr 28, 2025

View reviewed changes

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java Outdated Show resolved Hide resolved

csviri changed the title ~~improve: blacklist of problematic resources for previous version annotation~~ improve: blocklist of problematic resources for previous version annotation Apr 29, 2025

csviri marked this pull request as ready for review April 29, 2025 13:00

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2025

xstefank requested changes Apr 29, 2025

View reviewed changes

metacosm requested changes Apr 30, 2025

View reviewed changes

csviri requested review from metacosm and xstefank May 8, 2025 07:43

xstefank approved these changes May 12, 2025

View reviewed changes

csviri added 6 commits May 14, 2025 09:23

feat: blacklist of problematic resources for previous version annotation

5e80d98

Signed-off-by: Attila Mészáros <[email protected]>

rename to blocklist, added javadocs

f7ba067

Signed-off-by: Attila Mészáros <[email protected]>

wip

3b147fe

Signed-off-by: Attila Mészáros <[email protected]>

remove deamonSet since was not able to reproduce the behavior

0aefb56

Signed-off-by: Attila Mészáros <[email protected]>

Add integration test

74c13ff

Signed-off-by: Attila Mészáros <[email protected]>

explanation and using Set instead of list

7a1902b

Signed-off-by: Attila Mészáros <[email protected]>

csviri force-pushed the blacklist-prev-annotation branch from 228f8d2 to 7a1902b Compare May 14, 2025 07:23

metacosm added 2 commits May 15, 2025 17:56

fix: improve javadoc

e31793c

Signed-off-by: Chris Laprun <[email protected]>

refactor: rename so that relation between methods is more explicit

19ce185

Signed-off-by: Chris Laprun <[email protected]>

csviri mentioned this pull request May 16, 2025

Fix infinite resource updates due empty EnvVars #2803

Merged

csviri commented May 16, 2025

View reviewed changes

naming

7639330

Signed-off-by: Attila Mészáros <[email protected]>

metacosm approved these changes May 16, 2025

View reviewed changes

metacosm merged commit e3c828f into main May 16, 2025
25 checks passed

metacosm deleted the blacklist-prev-annotation branch May 16, 2025 13:56

improve: blocklist of problematic resources for previous version annotation #2774

improve: blocklist of problematic resources for previous version annotation #2774

Uh oh!

Conversation

csviri commented Apr 28, 2025

Uh oh!

Uh oh!

csviri commented Apr 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csviri Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csviri May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csviri commented May 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

csviri Apr 29, 2025 •

edited

Loading

csviri May 12, 2025 •

edited

Loading