fix(validation): Remove default channel validation from bundle lib#408
Conversation
openshift-ci-robot
added
the
approved
It is not possible to determine if the default channel value is valid or not given we can "infer" default channel by using an emptry string. Also, author can add a default channel that is not listed in the channel list in some cases. As a result, the default channel is subjective and there is no methodology to validate it. Therefore, removing this validation will avoid any false positve. Signed-off-by: Vu Dinh <vdinh@redhat.com>
dinhxuanvu
force-pushed
the
remove-defaultchan-validation
branch
from
8bb774f to
9228b2b
Compare
| fileContent += fmt.Sprintf("LABEL %s=%s\n", MetadataLabel, metadata) | ||
| fileContent += fmt.Sprintf("LABEL %s=%s\n", PackageLabel, packageName) | ||
| fileContent += fmt.Sprintf("LABEL %s=%s\n", ChannelsLabel, channels) | ||
| fileContent += fmt.Sprintf("LABEL %s=%s\n\n", ChannelDefaultLabel, chanDefault) |
There was a problem hiding this comment.
If channelDefault is an empty string, this label will be empty. Is that ok/desirable?
There was a problem hiding this comment.
I think it is OK to not have a defaultChannel value AKA empty string because in annotation generation function, there is a case where inferred defaultChannel is set to empty string. @Bowenislandsong Can you double-confirm this?
There was a problem hiding this comment.
Wouldn't it be better to just not set this label at all?
There was a problem hiding this comment.
yes. Later when added to Index, the default behavior will kick in (right now the default behavior is to add the first channel as default).
There was a problem hiding this comment.
In that case, wouldn't it be better/more obvious to the user to write a label containing the first channel as default if a default isn't explicitly set? https://github.com/operator-framework/operator-registry/pull/408/files#r462470459 implies that this isn't desirable.
| func GenerateDockerfile(mediaType, manifests, metadata, copyManifestDir, copyMetadataDir, workingDir, packageName, channels, channelDefault string) ([]byte, error) { | ||
| var fileContent string | ||
|
|
||
| chanDefault, err := ValidateChannelDefault(channels, channelDefault) |
There was a problem hiding this comment.
If channelDefault is not an empty string, we still want to verify that it exists in channels.
There was a problem hiding this comment.
There are cases when that is totally fine to have default channel not on the list. @shawn-hurley Can you explain this further?
There was a problem hiding this comment.
Default channel mentioned in the bundle != Default channel on the package
estroz
left a comment
estroz
left a comment
There was a problem hiding this comment.
Assuming opm libraries check if a specified default channel exists in some packaged bundle
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dinhxuanvu, estroz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6 participants
It is not possible to determine if the default channel value is valid
or not given we can "infer" default channel by using an emptry string.
Also, author can add a default channel that is not listed in the channel
list in some cases. As a result, the default channel is subjective
and there is no methodology to validate it. Therefore, removing this
validation will avoid any false positve.
Signed-off-by: Vu Dinh vdinh@redhat.com
Description of the change:
Motivation for the change:
Reviewer Checklist
/docs