-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
⚠️ Upgrade resource to keep more and better structured information (go/v3-alpha) #1869
Conversation
77f8f40
to
c9146c8
Compare
c9146c8
to
79888fc
Compare
79888fc
to
6d37b49
Compare
361800b
to
5d173e4
Compare
c85aa06
to
852a240
Compare
Some comments:
|
// Check if we need to do the API and controller before updating with previously existing info | ||
doAPI := s.resource.API != nil && s.resource.API.Version != "" | ||
doController := s.resource.Controller | ||
|
||
if doAPI { | ||
// Update the known data about resource | ||
if _, err := s.config.UpdateResources(s.resource); err != nil { | ||
return fmt.Errorf("error updating resources in config: %w", err) | ||
} |
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.
// Check if we need to do the API and controller before updating with previously existing info | |
doAPI := s.resource.API != nil && s.resource.API.Version != "" | |
doController := s.resource.Controller | |
if doAPI { | |
// Update the known data about resource | |
if _, err := s.config.UpdateResources(s.resource); err != nil { | |
return fmt.Errorf("error updating resources in config: %w", err) | |
} | |
if _, err := s.config.UpdateResources(s.resource); err != nil { | |
return fmt.Errorf("error updating resources in config: %w", err) | |
} | |
if s.doAPI { | |
We need to update the resource
always because we add the GKV in all scenarios. Then, it should be the first step. The doAPI := s.resource.API != nil && s.resource.API.Version != ""
added here not make clear that we always update the resource for all scenarios and make very confuse know when it should scaffold an API.
Then, doController := s.resource.Controller
shows not required either. Why we are not using here the s.doController as before?
However, IMO we should NOT do this changes for v2 since it adds unnecessary risk. Could we try to revert the changes here and avoid perform changes in the logic of v2 plugin unless it is required for we achieve its goal?
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.
doAPI := s.resource.API != nil && s.resource.API.Version != ""
is the condition that has to be true in case we scaffold the resource:
As I already explained in #1869 (comment), s.resource.API
can't be a nil pointer so we can't just use doAPI := s.resource.API != nil
, we also have to check if the string is empty or not. Because if you go to Options.NewResource
, you will see that this string is only set if Options.DoAPI
is true.
doAPI
and doController
need to be considerd before updating:
Imagine that we first do a kb create api ... --resource --controller=false
. Afterwards, we scaffold the controller with kb create api ... --resource=false --controller
. One you update the resource, s.resource.API != nil && s.resource.API.Version != ""
will be true because we already scaffolded this in the previous command, and therefore the second command call will think that he needs to scaffold the API again, resulting in errors because some files yield errors if they try to be scaffolded and they previously exist. This is the reason why we have to remember doAPI
and doController
before updating the resource.
Right now, v2 only updates the resource if you set the --resource
flag, and thats exactly the behavior I left. Your request to move this out of the conditional would be the change that is not required and will mess with the v2 project.
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.
As I already explained in #1869 (comment), s.resource.API can't be a nil pointer
My point here is that IMO we should still using here the bool doAPI as it was before. The changes made here does not make the code better. Could we revert and keeps the original logic with the only changes that matter for us which is moving theupdateResources
to the top since now we are always updating the resources?
Also, not that this logic should not change at all for v2. I mean, the update resources for v2 should still at the same line where it is currently in master. The purpose of this PR is to change the behaviour only for v3.
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.
Hi @Adirio,
I tried to do a good review here and cover all by once. Could you please check the comments? After we address them, I am happy to approve this one.
9ab9463
to
3d660c4
Compare
I solved a couple comments, explained others (most of the times the same explanation is valid for multiple comments so I just linked it so that the conversation is kept in one place only) and left only the |
} | ||
|
||
// HasGroup returns true if group is already tracked | ||
func (c Config) HasGroup(group string) bool { | ||
// Return true if the target group is found in the tracked resources | ||
for _, r := range c.Resources { | ||
if strings.EqualFold(group, r.Group) { | ||
if strings.EqualFold(group, r.QualifiedGroup()) { |
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.
IMO we cannot do it. It changes the default behaviour. We do not use the qualified group (with the domain) to do this check currently. And then, IMO we should not introduce this new logic/rule here. It has a big chance to impacts the v2 behaviours and introduces a braking change besides not be part of the scope of this pr.
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.
It doesn't because for v2 project configuration, domain is not stored in the Resources []*Resource
field, so the QualifiedGroup
is the same as the group.
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 susceptible to errors and changes behavioural. In addition, the HasGrupo function should not use the domain. As the name says it should only and check if the group exists or not.
c/c @estroz
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.
It sounds like @Adirio is explicitly saying that behavior isn't changing by using Resource.QualifiedGroup()
here, because v2 configs consider a qualified group to be just Resource.Group
in the context of that resource.
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.
The func is called HasGroup which returns true if the group is already tracked
. However, its code is checking the Group + Domain which theoretically apparently shows fine at least here and now, because we believe that the domains for v2 will be empty always.
However, note that in a follow up a contributor can for some reason starts to set the domain before that such as was done in v3 workaround (see #1869 (review)) and then, it will starts to fails or indeed worst will fail in some specific scenarios. Also, I do not think that we should change the logic and how the business rules are in place for v2 unless it is really required to solve a problem/requirement which is not this case. Otherwise, we are assuming a risk completed unnecessarily.
The CI does not cover all possible scenarios and also not cover its integration with other projects that might have been consuming it as SDK. E.g https://github.com/operator-framework/operator-sdk/blob/master/internal/plugins/ansible/v1/scaffolds/api.go#L89
How a consumer of this pkg will know that the HasGroup is not checking so longer the group? Would all consumers also require put in place the conditionals/workarounds done here for v2/v3 to make these changes work and keep in mind that they cannot fill the domain?
c/c @estroz
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 don't believe that the domain is empty, we enforce it. The domain in v2 project files is enforced to be empty, and therefore the qualified group in this context is only the group, which matches with the GVK
generated by v2 options. Bear in mind that it is calling the qualified group of each of the itmes in Config.Resources
, not in the actual resource.
} | ||
} | ||
return resource.GVK{ | ||
Group: opts.QualifiedGroup(), |
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.
It is wrong. It should not use the QualifiedGroup at all. GKV should not contain the domain info.
Also, it has a big chance to impact the v2/ behaviour.
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.
Again not true. GVK includes the domain info: proof.
We weren't considering it in v2 project files but thats why there is a conditional above that uses the Group
without the Domain
.
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.
In the Kubebuilder and in the solution of this PR resource.GVK
represents:
resources:
- group: cache
kind: Memcached
version: v1alpha1
The Options
represent the flags create api --group --version --kind
.
Then, Group: opts.QualifiedGroup(),
is at least very confusing. In a moment a Group only represents the group informed in another moment we are defining that a Group is the QualifiedGroup. IHMO we do need to use QualifiedGroup
because it affects the rules in place. Also, see that the changes here affect its consumers.
c/c @estroz
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.
GVK is a k/k concept and it uses the group with the domain. The division into group and domain is something artificial that we made out to avoid repetition (prevent the user to have to specify the domain every command call). As we are gonna be storing not only internal resources (core builtin types are external for example) we need to use the real GVK as the key. Before, only local resources whwere stored in the config file so the partial key that we were using was enough to uniquely identify each resource but not any more, the domain is needed, adn thus is why the qualified group name is required.
3d660c4
to
b000deb
Compare
Signed-off-by: Adrian Orive Oneca <adrian.orive.oneca@gmail.com>
b000deb
to
4a9906b
Compare
fs.BoolVar(&p.runMake, "make", true, "if true, run make after generating files") | ||
p.options = &goPlugin.Options{} | ||
fs.StringVar(&p.options.Group, "group", "", "resource Group") | ||
p.options.Domain = p.config.Domain |
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.
p.options.Domain = p.config.Domain |
We should NOT need to set this logic here. And then, if we remove this line all stops to work. If we try to add this logic inside of the newResource where it shows more appropriated the tooling will still not working which lead to me think that it is a workaround to make the changes made work and that we have a design issue.
c/c @estroz
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.
Why is setting an options field from a config field a design failure? Options holds everything that will be needed to create a resource, and the domain is needed. It also offers a clear rute for when we provide this as an flag for users, which will happen once we accept external resources.
@Adirio: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Changes
groupversion_info.go
files to contain the domain too because we are supporting empty groups, which leave a strange comment:pkg/plugins/golang/v3/scaffolds/internal/templates/api/group.go
andtestdata/project-v3**/groupversion_info.go
Resource
andConfig
models: remaining files.Description
Upgrade
Resource
model towards a unified internal and external (config file) representation.Legend:
Related topics