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

add new optional for bundle validate #4693

Merged

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Mar 24, 2021

Description of the change:

Screen Shot 2021-03-24 at 01 43 43

Screen Shot 2021-03-24 at 01 44 06

Motivation for the change:

Allow SDK to use the new checks added for OperatorHub.io

NOTE This PR is using my fork. We need to get merged first: operator-framework/api#103 and follow-up: operator-framework/api#111

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 24, 2021
@camilamacedo86 camilamacedo86 temporarily deployed to deploy March 24, 2021 02:01 Inactive
@camilamacedo86 camilamacedo86 temporarily deployed to deploy March 24, 2021 02:01 Inactive
@camilamacedo86 camilamacedo86 temporarily deployed to deploy March 24, 2021 02:01 Inactive
@camilamacedo86 camilamacedo86 temporarily deployed to deploy March 24, 2021 02:01 Inactive
@camilamacedo86 camilamacedo86 temporarily deployed to deploy March 24, 2021 02:01 Inactive
@camilamacedo86 camilamacedo86 temporarily deployed to deploy March 24, 2021 02:01 Inactive
@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 24, 2021
@camilamacedo86 camilamacedo86 temporarily deployed to deploy March 24, 2021 02:27 Inactive
@camilamacedo86 camilamacedo86 temporarily deployed to deploy March 24, 2021 02:27 Inactive
@camilamacedo86 camilamacedo86 temporarily deployed to deploy March 24, 2021 02:27 Inactive
@camilamacedo86 camilamacedo86 temporarily deployed to deploy March 24, 2021 02:27 Inactive
@camilamacedo86 camilamacedo86 temporarily deployed to deploy March 24, 2021 02:27 Inactive
@camilamacedo86 camilamacedo86 temporarily deployed to deploy March 24, 2021 02:27 Inactive
@estroz
Copy link
Member

estroz commented Mar 24, 2021

I'm not sure I see the value in supplying an optional value. Shouldn't the CSV validator easily be able to perform some checks if a CSV value is present, and skip them if not present?

@camilamacedo86 camilamacedo86 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 26, 2021
@camilamacedo86 camilamacedo86 force-pushed the api-new-checks branch 2 times, most recently from f767e06 to 2f16a16 Compare April 7, 2021 13:29
@camilamacedo86 camilamacedo86 removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 7, 2021
@camilamacedo86 camilamacedo86 marked this pull request as ready for review April 7, 2021 18:09
@estroz

This comment has been minimized.

@camilamacedo86

This comment has been minimized.

@camilamacedo86

This comment has been minimized.

@camilamacedo86

This comment has been minimized.

@camilamacedo86
Copy link
Contributor Author

HI @estroz, @jmrodri, @fabianvf

Now we have here only the validator impl.

Copy link
Member

@jmrodri jmrodri 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 Apr 13, 2021
@estroz
Copy link
Member

estroz commented Apr 13, 2021

Before this goes in, I would really like to see docs about how this value is used in the api repo, and have them referenced in SDK docs. Otherwise we're just adding some nebulous, obscure feature.

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 13, 2021
@marc-obrien
Copy link

/hold totally agree with Eric's points

@camilamacedo86

This comment has been minimized.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 14, 2021
Signed-off-by: Camila Macedo <cmacedo@redhat.com>
```sh
operator-sdk bundle validate ./bundle --select-optional suite=operatorframework --optional-values=k8s-version=1.22
```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@estroz @jmrodri @fabianvf @marc-obrien I added a note here as well.

@camilamacedo86
Copy link
Contributor Author

Hi @estroz, @jmrodri, @fabianvf;

Please, let me know if has anything else that we ought to address.

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.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2021
@camilamacedo86 camilamacedo86 merged commit 00be8dd into operator-framework:master Apr 20, 2021
@camilamacedo86 camilamacedo86 deleted the api-new-checks branch April 20, 2021 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants