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
CLID-10: Generate CatalogSource files #784
Conversation
@sherine-k: This pull request references CLID-10 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
@sherine-k: This pull request references CLID-10 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
@sherine-k: This pull request references CLID-10 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
@sherine-k: This pull request references CLID-10 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
@sherine-k: This pull request references CLID-10 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
1 similar comment
@sherine-k: This pull request references CLID-10 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
dd13288
to
a3e0b53
Compare
@sherine-k: This pull request references CLID-10 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
@sherine-k: This pull request references CLID-10 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
@sherine-k: This pull request references CLID-10 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
/jira refresh |
@sherine-k: This pull request references CLID-10 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
/hold |
pathComponents := strings.Split(catalogSpec.PathComponent, "/") | ||
catalogRepository := pathComponents[len(pathComponents)-1] | ||
catalogSourceName := "cs-" + catalogRepository + "-" + csSuffix | ||
errs := validation.IsDNS1035Label(catalogSourceName) |
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.
good catching finding this validation
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.
Agreed !!!
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.
Agreed !!!
return "" | ||
} | ||
|
||
func (o *ClusterResourcesGenerator) generateCatalogSource(catalogRef string, catalogSourceTemplateFile string) error { |
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 really like the validations happening on RFC here.
@@ -276,7 +276,19 @@ func getRelatedImageByDefaultChannel(log clog.PluggableLoggerInterface, olm []v1 | |||
if bundles[obj.Name] { | |||
log.Debug("config bundle: %d %v", i, obj.Name) | |||
log.Trace("config relatedImages: %d %v", i, obj.RelatedImages) | |||
relatedImages[obj.Name] = obj.RelatedImages | |||
riList := relatedImages[obj.Name] |
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 change needs to be on CLID-14 so I would prefer to merge CLID-10 first.
toBeAdded := false | ||
if bundles[obj.Name] && !pkg.Full { | ||
log.Debug("config bundle: %d %v", i, obj.Name) | ||
log.Trace("config relatedImages: %d %v", i, obj.RelatedImages) | ||
relatedImages[obj.Name] = obj.RelatedImages | ||
toBeAdded = true | ||
} | ||
// add all bundles | ||
if pkg.Full { | ||
relatedImages[obj.Name] = obj.RelatedImages | ||
if pkg.Full || toBeAdded { | ||
riList := relatedImages[obj.Name] | ||
if riList == nil { | ||
riList = []v1alpha3.RelatedImage{} | ||
} | ||
for _, ri := range obj.RelatedImages { | ||
if ri.Image == obj.Image { | ||
ri.Type = v1alpha2.TypeOperatorBundle | ||
} else { | ||
ri.Type = v1alpha2.TypeOperatorRelatedImage | ||
} | ||
riList = append(riList, ri) | ||
} | ||
relatedImages[obj.Name] = riList |
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 change needs to be on CLID-14 so I would prefer to merge CLID-10 first.
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.
/lgtm
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.
@sherine-k - as usual great work
I left some nits about the messaging
pathComponents := strings.Split(catalogSpec.PathComponent, "/") | ||
catalogRepository := pathComponents[len(pathComponents)-1] | ||
catalogSourceName := "cs-" + catalogRepository + "-" + csSuffix | ||
errs := validation.IsDNS1035Label(catalogSourceName) |
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.
Agreed !!!
pathComponents := strings.Split(catalogSpec.PathComponent, "/") | ||
catalogRepository := pathComponents[len(pathComponents)-1] | ||
catalogSourceName := "cs-" + catalogRepository + "-" + csSuffix | ||
errs := validation.IsDNS1035Label(catalogSourceName) |
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.
Agreed !!!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lmzuccarelli, sherine-k The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
CLID-10: targetCatalog replaces deprecated field targetName(removed) for GetUniqueName. CLID-10,CFE-824: Add updateStrategy to generated catalog source Failover to regular catalogSource generation if template generation fails V2 - Modify naming convention for catalogSource
/unhold |
/unhold |
@sherine-k: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
/lgtm |
/label acknowledge-critical-fixes-only |
CLID-10: targetCatalog replaces deprecated field targetName(removed) for GetUniqueName. CLID-10,CFE-824: Add updateStrategy to generated catalog source Failover to regular catalogSource generation if template generation fails V2 - Modify naming convention for catalogSource
[ART PR BUILD NOTIFIER] This PR has been included in build oc-mirror-plugin-container-v4.16.0-202401311812.p0.g3b94f36.assembly.stream for distgit oc-mirror-plugin. |
By looking at the PR, I can see that the CatalogSourceTemplate that pretty much anything in the CatlogSource metadata and spec can be updated except for 4 fields:
It would be helpful if the contract for "what" in the template is designed to be templatable vs. not is clearly documented. Especially hard-coding the namespace and format of the auto-generated name. I personally think those options should be overridable, but in my use cases, I would treat the generated CatalogSource as itself a template anyway, using Kustomize to override the namespace and name to my own purposes. |
Description
This PR contains implementation for CLID-10 :
targetCatalogSourceTemplate
to the imageSetConfig, in order to allow the user customization of the catalogSourcetargetName
andoriginalRef
from the imageSetConfigFixes # CLID-10.
PS: catalogSource resource name and filename use a different naming convention than v1:
"cs-" + catalogRepository + "-" + date
. We might need to reconsider this strategy based on how customers use these resources in the cluster. ie. Do they deploy the new catalogSource and delete the old? do they expect an update of the existing resource?Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Prior to testing, create the file clid10.yaml with content:
And the catalog-source_template.yaml
Next, perform a mirrorToDisk + DiskToMirror
Expected Outcome
The catalogSource file is generated under the working directory, under subfolder
cluster-resources
. The catalog source file name is derived from the catalog name, withcs-
as prefix, and the date as suffix.Ex: cs-redhat-operator-index-2024-01-22t17-37-25z.yaml