Skip to content
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

[RHPAM-2658] Define value as byte #349

Merged
merged 4 commits into from Jan 29, 2020

Conversation

ruromero
Copy link
Member

Signed-off-by: Ruben Romero Montes rromerom@redhat.com

Fix https://issues.redhat.com/browse/RHPAM-2658

I have reproduced the issue and validated the fix on the following environment

Server Version: 4.4.0-0.nightly-2020-01-23-054055
Kubernetes Version: v1.17.1
oc create -f deploy/catalog_resources/redhat/1.4.0/businessautomation-operator.1.4.0.clusterserviceversion.yaml                                                                                                  
The ClusterServiceVersion "businessautomation-operator.1.4.0" is invalid: spec.customresourcedefinitions.owned.specDescriptors.value: Invalid value: "": spec.customresourcedefinitions.owned.specDescriptors.value in body must be of type byte: ""

Signed-off-by: Ruben Romero Montes <rromerom@redhat.com>
@tchughesiv
Copy link
Member

/hold

@tchughesiv
Copy link
Member

@ruromero please try to persist this change in tools/csv-gen

Value: util.RawMessagePointer("false"),

Signed-off-by: Ruben Romero Montes <rromerom@redhat.com>
@ruromero
Copy link
Member Author

@tchughesiv I preferred to do it when the value is generated from a string to make it more reusable

@madorn
Copy link

madorn commented Jan 29, 2020

@ruromero @tchughesiv - hi folks, just a note that just discovered boolean switch will automatically set this false by default (unless otherwise set to "true" in the CR). It shouldn't be necessary to explicitly set false.

Check out this link: https://github.com/openshift/console/blob/master/frontend/packages/operator-lifecycle-manager/src/components/descriptors/reference/reference.md#4-booleanswitch

@tchughesiv
Copy link
Member

thanks @madorn ... thoughts no longer setting to false @ruromero ?

@ruromero
Copy link
Member Author

We can remove the flags but keep the change in the generator. Does that sound right?

@ruromero ruromero closed this Jan 29, 2020
@ruromero ruromero reopened this Jan 29, 2020
Signed-off-by: Ruben Romero Montes <rromerom@redhat.com>
@bmozaffa
Copy link
Collaborator

Right, though I don't see anything wrong with providing default values either... if anything, it's maybe more explicit and obvious to someone reviewing it.

Signed-off-by: Ruben Romero Montes <rromerom@redhat.com>
@tchughesiv
Copy link
Member

/lgtm
/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ruromero, spolti, tchughesiv

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tchughesiv
Copy link
Member

/hold cancel

@openshift-merge-robot openshift-merge-robot merged commit 6596848 into kiegroup:master Jan 29, 2020
@madorn
Copy link

madorn commented Jan 30, 2020

just discovered that completely omitting value will show true on UI if using 4.2.

It will default to false on 4.3 and 4.4

After discussing with UI team, setting the value here is discouraged. We are in the process of getting the docs updated.

In the meantime, completely omit the value field.

To influence the value you have two options:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants