-
Notifications
You must be signed in to change notification settings - Fork 78
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
Conversation
/assign @dinhxuanvu |
@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. |
There was a problem hiding this 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)} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙃
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dinhxuanvu updated, PTAL
@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)} |
There was a problem hiding this comment.
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.
@dinhxuanvu updated, ptal |
/lgtm |
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: