feat(olm-catalog) : add missing fields with placehoders in the CSV

[camilamacedo86]

Description of the change:
Add warn for spec.icon.

Motivation for the change:

Closes:

• Add CSV fields expected by various validator tools #2071

[estroz]

We need to make sure the user has replaced these field's text, which we are not doing in these changes in getEmptyRequiredCSVFields(). I suggest adding mandatory field checks in getEmptyRequiredCSVFields() and not setting these to placeholder values in setCSVDefaultFields(), since the former function will pass on placeholder data when it should not.

[camilamacedo86]

Hi @estroz,

Really tks for your review.

The getEmptyRequiredCSVFields() as you suggested by you will not add the fields. It will just show a warning message when the CSV file generated as the following which in POV is not a good experience for the users at all. Note that we can find issues caused by it.

Fill in the following required fields in file deploy/olm-catalog/project-test/0.0.1/project-test.v0.0.1.clusterserviceversion.yaml:

I mean: As I developer, I would like to generate the CSV with all fields which are required for the project works successfully in OLM and be accepted on it. Is it make sense?

[estroz]

@camilamacedo86 passing gen-csv or deployment with placeholder values is not the correct way to resolve these outstanding issues. Either the warning messages need to be more clear, or we need to error out. I think the validation library being worked on in operator-framework/api will help with this.

Furthermore I'm fairly certain operator-framework/getting-started#43 is caused by an RBAC issue and not minikube version. I've seen the minikube version status before on a successfully deployed CSV.

[camilamacedo86]

/test e2e-aws-go
/test e2e-aws-ansible
/test e2e-aws-subcommand

[camilamacedo86]

Hi @joelanford and @dmesser,

@estroz asked for I raise you both here and ask your inputs as well.

[joelanford]

If I'm understanding @estroz's comments correctly, I'm in agreement with him. If we're trying to solve the problem of helping the user generate a valid CSV, using placeholders won't help because they will obscure the fact that a particular field is not populated correctly.

I agree that the static bundle validation library is the correct solution here.

[camilamacedo86]

HI @estroz and @joelanford,

It was changed as we spoke. Then, we are here just adding icon.spec which was missing in the WARN which shows enough so far for we attend the community request. Are you ok with? Could we move forward with this PR?

[estroz]

@camilamacedo86 you could change this line to use log.Errorf() since these fields are required.

[camilamacedo86]

Hi @estroz,

@camilamacedo86 you could change this line to use log.Errorf() since these fields are required.

The WARN appears when we exec the command to generate the file. So, IMO shows an ERROR message by default is a really bad user experience and will be very confusing. I do not think that we should do it.

[camilamacedo86]

Hi @estroz,

The docs are updated as well as we spoke about it. Please, let me know if you are ok with.

[camilamacedo86]

/test images
/test e2e-aws-helm
/test e2e-aws-go
/test e2e-aws-ansible
/test e2e-aws-subcommand

[joelanford]

@estroz are we planning to replace this logic with the static bundle validator? If so, I would only recommend that we make this change if the static bundle validator is also going to include this check.

I'm pretty sure spec.icon is optional for OLM purposes, right? We probably need to distinguish optional and required fields. As is, this PR is grouping spec.icon in with other required fields and I'm not sure that's the impression we want to give to users.

Thoughts?

[joelanford]

If we do go forward with this PR, we should be explicit about which image types are supported.

But, let's hold off on further changes until @estroz weighs in on my previous question so we don't use CI resources unnecessarily.

[camilamacedo86]

Hi @joelanford,

I'm pretty sure spec.icon is optional for OLM purposes, right?

Currently, the WARN is done for:

INFO[0000] Required csv fields not filled in file deploy/olm-catalog/app-operator/0.0.1/app-operator.v0.0.1.clusterserviceversion.yaml:
spec.keywords
spec.maintainers
spec.provider

In my understanding, the spec.icon added here is so "optional" as the above specs keywords, maintainers, provider. So, the operator will work without all this info, however, they are required to be populated in order to integrate with OLM and distribute the solution in OperatorHub and/or OCP catalogue.

I understand that all this validation will be improved in the future with new features as we spoke about. It should be just a small pr to help users until there. Shows that we will know better how we can go forward when the operator-framework/api#3 be done.

I am looking for your positions. @joelanford and @estroz
Realy tks for your inputs.

[estroz]

@joelanford I was under the impression that spec.icon is not required. According to #2071 that might not be the case, or there is a misunderstanding about what is "required" by OLM or not. I will confirm with the OLM team.

The api library will perform these checks.

EDIT: spec.icon is not required. operator-courier issues a warning if it is not present.

[camilamacedo86]

Hi @estroz @joelanford,

It is not clear for me the comments. Could we not move forward with this small one? What you are not ok with here and should be changed? Or you would like to close this one?

[joelanford]

Alright, so my high-level understanding is that the validations done by operator-courier will be replaced with the validations performed by the static bundle validation library.

Since our current CSV generation code does not distinguish between required and recommended fields and since we'll be migrating to use the static bundle validation library, my vote would be to close this PR and update #2071 to reflect our plans to solve it via with the validation library.

@estroz Once the validation library is merged, we plan to integrate it into a new scorecard test. Will we also integrate it into gen-csv?

[camilamacedo86]

Closing this one after the discussions and definitions over how it will be addressed.

[estroz]

@joelanford yes it will be.