Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
List inbuilt sources if CRD access is restricted #948
List inbuilt sources if CRD access is restricted #948
Changes from 10 commits
a0e527b
3f36d7a
30a33ad
0a8e024
eb264bc
4ee87b1
5dc4893
eeee24b
381befd
eb58eb4
67eadfe
ac9988a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'm not sure whether we should set or not-set the GVK based on the result. This might lead to unexpected behaviour depending on the context where you run it. I think its better to just leave out the GVK on the list (or maybe even better, but not sure if this works), just return a slice of Unstructed instead of an UnstructuredList
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 was done to show proper values if json or yaml format of list is requested and the list contains multiple types of source objects.
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.
We'd need GVK set for the (list) object here as (json/yaml) printers require it. The question was which version/group to set if there are different types of sources found, so we set cleared group and version values and set
List
for kind.If there is only one type of source object found OR user requested single type using
--type
filter, we keep the GVK intact.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.
Yes, but imagine the situation that by accident you have only one source of one type. If you list those, you the GVK set. Some time later another source of another type is added. Suddenly know the GVK gets removed or changed to an artificial "List". I think as we are dealing with a heterogenous list, that, by accident, can be also homogenous, we should treat it as a heterogenous list always. I would not mind to introduce a new type here, like we did for the
Export
, with anclient.knative.dev
group and av1alpha1
version for now, but then use it all the time.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.
yea, we could do that, however for the other point about list of single source type, I think setting exact source type for single-type-of-source list is explicit and we could scope the client custom type for list only if there multiple types of sources.
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.
Added #953 to track adding setting this custom client type for list. We can refine in next iteration.
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.
I wonder what is the best function behavior when
gvks
isnil
: a) to return anil
list or b) to ignoregvks
and usetypes
to filter. The current behavior is a).Yet when
types
isnil
, the current behavior is b) to ignoretypes
and usegvks
to filter, not a) to return anil
list. It makes me a little weird.Of course it doesn't affect the current functions. For future usage, maybe change to the same behavior when either of these two parameters is
nil
.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.
We use
types
only to subset the found source types. If gvks list is nil, it returns nil. Does this answer your question ?