feat: split name into bundle and csv names to avoid confusion

Fixes #738

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

Author: metacosm

annotations/src/main/java/io/quarkiverse/operatorsdk/annotations/CSVMetadata.java

@@ -21,9 +21,29 @@
      * gradle) name. This name can be used to assign reconcilers to the same bundle by creating a {@link SharedCSVMetadata}
      * implementation bearing a {@link CSVMetadata} annotation specifying the CSV metadata to be shared among reconcilers
      * assigned to that named bundle.
+     *
+     * @deprecated Use {@link #bundleName()} and {@link #csvName()} instead as previously this method was being used for both
+     *             values resulting in confusion or even problems setting the correct CSV name as recommended by OLM. See
+     *             <a href='https://github.com/quarkiverse/quarkus-operator-sdk/issues/738'>this issue</a> for a more detailed
+     *             discussion.
      */
+    @Deprecated
     String name() default "";
 
+    /**
+     * The name which should be used for the generated bundle. If not provided, the name is derived from the project's (maven or
+     * gradle) name. This name can be used to assign reconcilers to the same bundle by creating a {@link SharedCSVMetadata}
+     * implementation bearing a {@link CSVMetadata} annotation specifying the CSV metadata to be shared among reconcilers
+     * assigned to that named bundle.
+     */
+    String bundleName() default "";
+
+    /**
+     * The name used in the CSV metadata stanza. If not provided, this will default to {@code <bundle name>.v<version>}
+     * as recommended by OLM.
+     */
+    String csvName() default "";
+
     /**
      * Extra annotations that should be added to the CSV metadata.
      */

bundle-generator/deployment/src/main/java/io/quarkiverse/operatorsdk/bundle/deployment/BundleGenerator.java

@@ -66,12 +66,12 @@ public static List<ManifestsBuilder> prepareGeneration(BundleGenerationConfigura
             var missing = addCRDManifestBuilder(crds, builders, csvMetadata, csvBuilder.getOwnedCRs());
             if (!missing.isEmpty()) {
                 throw new IllegalStateException(
-                        "Missing owned CRD data for resources: " + missing + " for bundle: " + csvMetadata.name);
+                        "Missing owned CRD data for resources: " + missing + " for bundle: " + csvMetadata.bundleName);
             }
             // output required CRDs in the manifest, output a warning in case we're missing some
             missing = addCRDManifestBuilder(crds, builders, csvMetadata, csvBuilder.getRequiredCRs());
             if (!missing.isEmpty()) {
-                log.warnv("Missing required CRD data for resources: {0} for bundle: {1}", missing, csvMetadata.name);
+                log.warnv("Missing required CRD data for resources: {0} for bundle: {1}", missing, csvMetadata.bundleName);
             }
         }
 
@@ -93,7 +93,7 @@ private static HashSet<String> addCRDManifestBuilder(Map<String, CRDInfo> crds,
 
     private static SortedMap<String, String> generateBundleLabels(CSVMetadataHolder csvMetadata,
             BundleGenerationConfiguration bundleConfiguration, Version version) {
-        var packageName = bundleConfiguration.packageName.orElse(csvMetadata.name);
+        var packageName = bundleConfiguration.packageName.orElse(csvMetadata.bundleName);
 
         SortedMap<String, String> values = new TreeMap<>();
         values.put(join(BUNDLE_PREFIX, CHANNEL, DEFAULT, ANNOTATIONS_VERSION),

bundle-generator/deployment/src/main/java/io/quarkiverse/operatorsdk/bundle/deployment/BundleProcessor.java

@@ -98,31 +98,31 @@ CSVMetadataBuildItem gatherCSVMetadata(KubernetesConfig kubernetesConfig,
                     // Check whether the reconciler must be shipped using a custom bundle
                     final var csvMetadataAnnotation = reconcilerInfo.classInfo()
                             .declaredAnnotation(CSV_METADATA);
-                    final var maybeCSVMetadataName = getCSVMetadataName(csvMetadataAnnotation);
-                    final var csvMetadataName = maybeCSVMetadataName.orElse(defaultName);
+                    final var sharedMetadataName = getBundleName(csvMetadataAnnotation, defaultName);
+                    final var isNameInferred = defaultName.equals(sharedMetadataName);
 
-                    var csvMetadata = sharedMetadataHolders.get(csvMetadataName);
+                    var csvMetadata = sharedMetadataHolders.get(sharedMetadataName);
                     if (csvMetadata == null) {
                         final var origin = reconcilerInfo.classInfo().name().toString();
 
-                        if (!csvMetadataName.equals(defaultName)) {
+                        if (!sharedMetadataName.equals(defaultName)) {
                             final var maybeExistingOrigin = csvGroups.keySet().stream()
-                                    .filter(mh -> mh.name.equals(csvMetadataName))
+                                    .filter(mh -> mh.bundleName.equals(sharedMetadataName))
                                     .map(CSVMetadataHolder::getOrigin)
                                     .findFirst();
                             if (maybeExistingOrigin.isPresent()) {
                                 throw new IllegalStateException("Reconcilers '" + maybeExistingOrigin.get()
                                         + "' and '" + origin
-                                        + "' are using the same bundle name '" + csvMetadataName
+                                        + "' are using the same bundle name '" + sharedMetadataName
                                         + "' but no SharedCSVMetadata implementation with that name exists. Please create a SharedCSVMetadata with that name to have one single source of truth and reference it via CSVMetadata annotations using that name on your reconcilers.");
                             }
                         }
                         csvMetadata = createMetadataHolder(csvMetadataAnnotation,
-                                new CSVMetadataHolder(csvMetadataName, defaultVersion, defaultReplaces, vcsUrl, origin));
+                                new CSVMetadataHolder(sharedMetadataName, defaultVersion, defaultReplaces, vcsUrl, origin));
                     }
                     log.infov("Assigning ''{0}'' reconciler to {1}",
                             reconcilerInfo.nameOrFailIfUnset(),
-                            getMetadataOriginInformation(csvMetadataAnnotation, maybeCSVMetadataName, csvMetadata));
+                            getMetadataOriginInformation(csvMetadataAnnotation, isNameInferred, csvMetadata));
 
                     csvGroups.computeIfAbsent(csvMetadata, m -> new ArrayList<>()).add(
                             augmentReconcilerInfo(reconcilerInfo));
@@ -192,14 +192,14 @@ private static void augmentResourceInfoIfCR(ReconciledAugmentedClassInfo<?> reco
         }
     }
 
-    private String getMetadataOriginInformation(AnnotationInstance csvMetadataAnnotation, Optional<String> csvMetadataName,
+    private String getMetadataOriginInformation(AnnotationInstance csvMetadataAnnotation, boolean isNameInferred,
             CSVMetadataHolder metadataHolder) {
         final var isDefault = csvMetadataAnnotation == null;
-        final var actualName = metadataHolder.name;
+        final var actualName = metadataHolder.bundleName;
         if (isDefault) {
             return "default bundle automatically named '" + actualName + "'";
         } else {
-            return "bundle " + (csvMetadataName.isEmpty() ? "automatically " : "") + "named '"
+            return "bundle " + (isNameInferred ? "automatically " : "") + "named '"
                     + actualName + "' defined by '" + metadataHolder.getOrigin() + "'";
         }
     }
@@ -295,22 +295,31 @@ private Map<String, CSVMetadataHolder> getSharedMetadataHolders(String name, Str
             if (csvMetadataAnn != null) {
                 final var origin = sharedMetadataImpl.name().toString();
                 final var metadataHolder = createMetadataHolder(csvMetadataAnn, csvMetadata, origin);
-                final var existing = result.get(metadataHolder.name);
+                final var existing = result.get(metadataHolder.bundleName);
                 if (existing != null) {
                     throw new IllegalStateException(
-                            "Only one SharedCSVMetadata named " + metadataHolder.name
+                            "Only one SharedCSVMetadata named " + metadataHolder.bundleName
                                     + " can be defined. Was defined on (at least): " + existing.getOrigin() + " and " + origin);
                 }
-                result.put(metadataHolder.name, metadataHolder);
+                result.put(metadataHolder.bundleName, metadataHolder);
             }
         });
         return result;
     }
 
-    private Optional<String> getCSVMetadataName(AnnotationInstance csvMetadataAnnotation) {
-        return Optional.ofNullable(csvMetadataAnnotation)
-                .map(annotation -> annotation.value("name"))
-                .map(AnnotationValue::asString);
+    private static String getBundleName(AnnotationInstance csvMetadata, String defaultName) {
+        if (csvMetadata == null) {
+            return defaultName;
+        } else {
+            final var bundleName = csvMetadata.value("bundleName");
+            if (bundleName != null) {
+                return bundleName.asString();
+            } else {
+                return Optional.ofNullable(csvMetadata.value("name"))
+                        .map(AnnotationValue::asString)
+                        .orElse(defaultName);
+            }
+        }
     }
 
     private CSVMetadataHolder createMetadataHolder(AnnotationInstance csvMetadata,
@@ -325,8 +334,8 @@ private CSVMetadataHolder createMetadataHolder(AnnotationInstance csvMetadata, C
         }
 
         final var providerField = csvMetadata.value("provider");
-        String providerName = null;
-        String providerURL = null;
+        String providerName = mh.providerName;
+        String providerURL = mh.providerURL;
         if (providerField != null) {
             final var provider = providerField.asNested();
             providerName = ConfigurationUtils.annotationValueOrDefault(provider, "name",
@@ -472,8 +481,9 @@ private CSVMetadataHolder createMetadataHolder(AnnotationInstance csvMetadata, C
         }
 
         return new CSVMetadataHolder(
-                ConfigurationUtils.annotationValueOrDefault(csvMetadata, "name",
-                        AnnotationValue::asString, () -> mh.name),
+                getBundleName(csvMetadata, mh.bundleName),
+                ConfigurationUtils.annotationValueOrDefault(csvMetadata, "csvName",
+                        AnnotationValue::asString, () -> mh.csvName),
                 ConfigurationUtils.annotationValueOrDefault(csvMetadata, "description",
                         AnnotationValue::asString, () -> mh.description),
                 ConfigurationUtils.annotationValueOrDefault(csvMetadata, "displayName",

bundle-generator/deployment/src/main/java/io/quarkiverse/operatorsdk/bundle/deployment/builders/CsvManifestsBuilder.java

@@ -54,7 +54,7 @@ public CsvManifestsBuilder(CSVMetadataHolder metadata, BuildTimeOperatorConfigur
 
         csvBuilder = new ClusterServiceVersionBuilder();
 
-        final var metadataBuilder = csvBuilder.withNewMetadata().withName(getName());
+        final var metadataBuilder = csvBuilder.withNewMetadata().withName(metadata.csvName);
         if (metadata.annotations != null) {
             metadataBuilder.addToAnnotations("olm.skipRange", metadata.annotations.skipRange);
             metadataBuilder.addToAnnotations("containerImage", metadata.annotations.containerImage);
@@ -72,7 +72,7 @@ public CsvManifestsBuilder(CSVMetadataHolder metadata, BuildTimeOperatorConfigur
         final var csvSpecBuilder = csvBuilder
                 .editOrNewSpec()
                 .withDescription(metadata.description)
-                .withDisplayName(defaultIfEmpty(metadata.displayName, getName()))
+                .withDisplayName(defaultIfEmpty(metadata.displayName, metadata.csvName))
                 .withKeywords(metadata.keywords)
                 .withReplaces(metadata.replaces)
                 .withVersion(metadata.version)

bundle-generator/deployment/src/main/java/io/quarkiverse/operatorsdk/bundle/deployment/builders/ManifestsBuilder.java

@@ -45,7 +45,7 @@ public abstract byte[] getManifestData(List<ServiceAccount> serviceAccounts, Lis
     public abstract String getManifestType();
 
     public String getName() {
-        return metadata.name;
+        return metadata.bundleName;
     }
 
     @Override

bundle-generator/runtime/src/main/java/io/quarkiverse/operatorsdk/bundle/runtime/BundleGenerationConfiguration.java

@@ -3,6 +3,7 @@
 import java.util.List;
 import java.util.Optional;
 
+import io.quarkiverse.operatorsdk.annotations.CSVMetadata;
 import io.quarkus.runtime.annotations.ConfigItem;
 import io.quarkus.runtime.annotations.ConfigRoot;
 
@@ -28,7 +29,10 @@ public class BundleGenerationConfiguration {
 
     /**
      * The name of the package that bundle belongs to.
+     *
+     * @deprecated Use {@link CSVMetadata#bundleName()} instead
      */
+    @Deprecated(forRemoval = true)
     @ConfigItem
     public Optional<String> packageName;
 

bundle-generator/runtime/src/main/java/io/quarkiverse/operatorsdk/bundle/runtime/CSVMetadataHolder.java

@@ -8,7 +8,8 @@
 
 public class CSVMetadataHolder {
     private static final Logger log = Logger.getLogger(CSVMetadataHolder.class.getName());
-    public final String name;
+    public final String bundleName;
+    public final String csvName;
     private final String origin;
     public final String description;
     public final String displayName;
@@ -120,27 +121,31 @@ public RequiredCRD(String kind, String name, String version) {
 
     }
 
-    public CSVMetadataHolder(String name, String version, String replaces, String providerURL, String origin) {
-        this(name, null, null, null, null, null, providerURL, replaces, null, version, null, null, null, null, null, null, null,
+    public CSVMetadataHolder(String bundleName, String version, String replaces, String providerURL, String origin) {
+        this(bundleName, null, null, null, null, null, null, providerURL, replaces, null, version, null, null, null, null, null,
+                null, null,
                 null,
                 origin);
     }
 
-    public CSVMetadataHolder(String name, String description, String displayName, Annotations annotations, String[] keywords,
+    public CSVMetadataHolder(String bundleName, String csvName, String description, String displayName, Annotations annotations,
+            String[] keywords,
             String providerName,
             String providerURL, String replaces, String[] skips, String version, String maturity,
             String minKubeVersion,
             Maintainer[] maintainers, Link[] links, Icon[] icon,
             InstallMode[] installModes, PermissionRule[] permissionRules, RequiredCRD[] requiredCRDs, String origin) {
-        this.name = name;
+        this.bundleName = bundleName;
+        assert version != null;
+        this.version = version;
+        this.csvName = csvName == null ? bundleName + ".v" + version : csvName;
         this.description = description;
         this.displayName = displayName;
         this.annotations = annotations;
         this.keywords = keywords;
         this.providerURL = providerURL;
         this.replaces = replaces;
         this.skips = skips;
-        this.version = version;
         this.maturity = maturity;
         this.minKubeVersion = minKubeVersion;
         this.maintainers = maintainers;
@@ -159,24 +164,28 @@ public CSVMetadataHolder(String name, String description, String displayName, An
                 providerName = userName;
                 msg = ". Using user name " + userName + " as default.";
             }
-            log.warnv("It is recommended that you provide a provider name provided for {0}{1}", name, msg);
+            log.warnv("It is recommended that you provide a provider name provided for {0}{1}", bundleName, msg);
         }
         this.providerName = providerName;
     }
 
+    /**
+     * CSVMetadataHolders are stored as key in Maps and identified by their associated bundle name, so that's what we use for
+     * equals and {@link #hashCode()}
+     */
     @Override
     public boolean equals(Object o) {
         if (this == o)
             return true;
         if (o == null || getClass() != o.getClass())
             return false;
         CSVMetadataHolder that = (CSVMetadataHolder) o;
-        return Objects.equals(name, that.name);
+        return Objects.equals(bundleName, that.bundleName);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(name);
+        return Objects.hash(bundleName);
     }
 
     public String getOrigin() {