Skip to content

Conversation

kevinrizza
Copy link
Member

This pr attempts to create parity in the API project with the existing operator-courier tool's validation command which validates package manifest format operator bundles. It does this only for the new operator bundle image directory format. It updates and resolves several issues with existing validators, as well as introduces a new optional validator for OperatorHub.io validation.

Additionally, it overhauls the internal test operator-verify command line tool to serve as an example for import implementation.

Pass by value to validator instead of pointer which ignores type
@kevinrizza kevinrizza requested a review from estroz June 8, 2020 14:12
@kevinrizza
Copy link
Member Author

kevinrizza commented Jun 8, 2020

Dependencies []*Dependency
}

func (b *Bundle) ObjsToValidate() []interface{} {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer the full name here for func method ObjectsToValidate for clarity.

Copy link
Member

@Bowenislandsong Bowenislandsong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Kevin, for finishing this work. I definitely was underestimating the amount of work here. I have gone through the list of validations and they all matches. Thank you.

Just comments centered around support for the incoming media types. I understand they are not supported by this validation library yet, but a small change might help next time when we have to update this. Thanks.

if b.foundCSV == false {
errs = append(errs, fmt.Errorf("unable to find a csv in bundle directory %s", b.dir))
} else if b.bundle == nil {
errs = append(errs, fmt.Errorf("unable to generate bundle from directory %s", b.dir))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
errs = append(errs, fmt.Errorf("unable to generate bundle from directory %s", b.dir))
errs = append(errs, fmt.Errorf("unable to load bundle from directory %s", b.dir))

Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far. A few comments.

}

if capability, ok := csv.ObjectMeta.Annotations["capabilities"]; ok {
validCapabilities := map[string]struct{}{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be clearer as a global variable.

errs = append(errs, fmt.Errorf("csv.Spec.Icon should only have one element"))
}

for _, icon := range csv.Spec.Icon {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should all icons be validated if there should be exactly one? Was this intended to be a no-op if len(csv.Spec.Icon) == 0?

if categories, ok := csv.ObjectMeta.Annotations["categories"]; ok {
categorySlice := strings.Split(categories, ",")

validCategories := map[string]struct{}{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global variable.

return errs
}

func checkAlmExamples(csv v1alpha1.ClusterServiceVersion) []error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check is already done in the CSV validator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

@anik120
Copy link
Contributor

anik120 commented Jun 12, 2020

@kevinrizza
From your PR commit message

It updates and resolves several issues with existing validators

Is is possible to enumerate the issues this PR attempts to solve for better summary?

@kevinrizza
Copy link
Member Author

@kevinrizza
From your PR commit message

It updates and resolves several issues with existing validators

Is is possible to enumerate the issues this PR attempts to solve for better summary?

@anik120 They are enumerated in the commit messages:

(fix) Actually validate csv fields

Pass by value to validator instead of pointer which ignores type
and
*Expose new bundle function to get all objects for default validators

Small enough changes that I didn't want to clutter the actual purpose of the pr description, but were needed in order to actually create parity.

@kevinrizza
Copy link
Member Author

Addressed everyone's feedback as much as possible, please take another look.

Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2020
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits

*Add new operatorhub.io validator
*Add parity for other default validators to operator-courier
*Modify verify manifest function to expose example validation call
*Expose new bundle function to get all objects for default validators
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2020
@kevinrizza
Copy link
Member Author

@estroz okay, updated once again based on feedback. thanks!

@estroz
Copy link
Member

estroz commented Jun 24, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2020
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@dinhxuanvu dinhxuanvu merged commit 8285761 into operator-framework:master Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants