adding logic to deployablebyolm to validate packagename #1167
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
do-not-merge/work-in-progress
|
from change #1167: |
acornett21
force-pushed
the
validate_packagename
branch
from
7479ccf to
8e57adc
Compare
acornett21
changed the title
acornett21
changed the title
|
from change #1167: |
| appName := packageName | ||
| if msgs := validation.IsDNS1035Label(packageName); len(msgs) != 0 { | ||
| logger.V(log.DBG).Info(fmt.Sprintf("package name %s does not comply with DNS-1035, prefixing to avoid errors", packageName)) | ||
| appName = "preflight-" + packageName |
There was a problem hiding this comment.
Why are we prefixing this here instead of just failing this check?
There was a problem hiding this comment.
We need the operator to still be able to be installed to avoid this error being thrown by k8s further along in the process.
There was a problem hiding this comment.
I'd go for a shorter prefix... pflt- or even p-. You don't want to possibly hit a long one and end up failing because it goes beyond 63.
There was a problem hiding this comment.
Either:
a) Make this prefix shorter and validate that it fixes the problem before use (by re-running the isDNS!035Label func again on the new value), or
b) Generate the name used to create the catalog source in a way that's reliably formed.
acornett21
force-pushed
the
validate_packagename
branch
2 times, most recently
from
c76c6e4 to
5b50039
Compare
|
from change #1167: |
… with DNS-1035 labeling, ensuring that CatalogSources can be created Signed-off-by: Adam D. Cornett <[email protected]>
acornett21
force-pushed
the
validate_packagename
branch
from
5b50039 to
d504f21
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
from change #1167: |
| appName := packageName | ||
| if msgs := validation.IsDNS1035Label(packageName); len(msgs) != 0 { | ||
| logger.V(log.DBG).Info(fmt.Sprintf("package name %s does not comply with DNS-1035, prefixing to avoid errors", packageName)) | ||
| appName = "p-" + packageName |
There was a problem hiding this comment.
This still needs to be re-run through the DNS label validation. Prefixing the value just fixes one possible invalid scenario.
There was a problem hiding this comment.
@komish Are you sure? In my testing, and our tests, this fixes both issues that are thrown. Also, if we re-validate, and it fails, then what do we do?
There was a problem hiding this comment.
What if the package is called 1app_package
There was a problem hiding this comment.
I suppose this is non-blocking. You're addressing a specific failure that exists, and what I'm asking is a potential (but non-existent) future state. We already have an example in the wild that needs this workaround, but we haven't had an example of what I'm asking you to do yet, and it's unlikely we would given certification would fail if the CatalogSource doesn't come up.
LGTM given
There was a problem hiding this comment.
I think if the above is the case, we'd want this to fail, since that name probably can't be used in other processes. Right now, we really are just trying to solve for the one case for 3scale.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: acornett21, bcrochet, komish, skattoju 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 |
Adding logic to
DeployableByOlmto validate that packagename complies with DNS-1035 labeling constraints.Appname was changed forOperatorGroup,CatalogSourceandSubscriptionto address cross referencing.Updating test/mock data.
Relates: operator 3scale-community-operator (0.11.0) community-operators-prod#4150
Testing