Skip to content

Approval: Clarify possible values #45

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

Merged
merged 1 commit into from
Aug 5, 2020

Conversation

alvaroaleman
Copy link
Contributor

Currently this field does not seem to get defaulted (should it?) and the docs are pretty useless because the name is self-explanatory for the what, but the how is not documented:

$ k explain subscription.spec.installPlanApproval
KIND:     Subscription
VERSION:  operators.coreos.com/v1alpha1

FIELD:    installPlanApproval <string>

DESCRIPTION:
     Approval is the user approval policy for an InstallPlan.

@alvaroaleman
Copy link
Contributor Author

/assign @dinhxuanvu

@alvaroaleman
Copy link
Contributor Author

@bipuladh @exdx @dinhxuanvu can one of you please review this? Its a tiny change that helps a lot with usability.

Also, it would be awesome if this repo would get an OWNERS file so it becomes clear who to assign changes to.

Copy link
Member

@exdx exdx left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I agree we need to have more clear CI around this repo - it's pretty new so we are still ironing out the details.

/lgtm

crds/zz_defs.go Outdated
@@ -98,7 +98,7 @@ func operatorsCoreosCom_catalogsourcesYaml() (*asset, error) {
return nil, err
}

info := bindataFileInfo{name: "operators.coreos.com_catalogsources.yaml", size: 7231, mode: os.FileMode(420), modTime: time.Unix(1591650274, 0)}
info := bindataFileInfo{name: "operators.coreos.com_catalogsources.yaml", size: 7231, mode: os.FileMode(436), modTime: time.Unix(1593116031, 0)}
Copy link
Member

Choose a reason for hiding this comment

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

this changed as a result of running codegen? A little confused as to why file permissions would change as a result of adding a comment to the source code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, as a result of make generate. I don't really understand this, I would not have expected this change but instead a change of the generated openapi schemes in crds/operators.coreos.com_installplans.yaml and crds/operators.coreos.com_subscriptions.yaml. To me, it looks like either I am using this somehow wrong or its broken. I was hoping there would be a job that fails and tells me what to do but doesn't seem to be the case 🙃

Copy link
Member

Choose a reason for hiding this comment

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

This is just a bit strange. Maybe you can discard these changes from the PR and just keep the main change in pkg/operators/installplan_types.go. Then I can lgtm and merge this PR. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dinhxuanvu updated, PTAL

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

@exdx can you advise on who to ping to get this merged?

crds/zz_defs.go Outdated
@@ -98,7 +98,7 @@ func operatorsCoreosCom_catalogsourcesYaml() (*asset, error) {
return nil, err
}

info := bindataFileInfo{name: "operators.coreos.com_catalogsources.yaml", size: 7231, mode: os.FileMode(420), modTime: time.Unix(1591650274, 0)}
info := bindataFileInfo{name: "operators.coreos.com_catalogsources.yaml", size: 7231, mode: os.FileMode(436), modTime: time.Unix(1593116031, 0)}
Copy link
Member

Choose a reason for hiding this comment

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

This is just a bit strange. Maybe you can discard these changes from the PR and just keep the main change in pkg/operators/installplan_types.go. Then I can lgtm and merge this PR. Thanks.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2020
@alvaroaleman
Copy link
Contributor Author

@dinhxuanvu updated, ptal

@dinhxuanvu
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2020
@dinhxuanvu dinhxuanvu merged commit b2e43de into operator-framework:master Aug 5, 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.

4 participants