-
Notifications
You must be signed in to change notification settings - Fork 0
feat: relations #303
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
base: main
Are you sure you want to change the base?
feat: relations #303
Conversation
4928320
to
c4e3060
Compare
c4e3060
to
ed11c85
Compare
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
19ea7a5
to
ad375ed
Compare
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
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 left some comments and suggestions.
gateway/schema/schema.go
Outdated
} | ||
|
||
// Initialize the relation enhancer after gateway is created | ||
g.relationEnhancer = NewRelationEnhancer(g) |
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's the same circular dependency as above. I don't really see a need to separate it in another struct. You don't have an possibility to add a different RelationEnhancer
so it makes no sense. I would move the methods to the Gateway
. You can however keep it in a separate file.
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.
You are not going to believe - I seen those deps, but decided to go with them since it is in the same package.
But eventually you have the point - no need to move it under a different struct.
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
listener/pkg/apischema/builder.go
Outdated
if schema.VendorExtensible.Extensions == nil { | ||
schema.VendorExtensible.Extensions = map[string]any{} | ||
} | ||
schema.VendorExtensible.Extensions[common.ScopeExtensionKey] = apiextensionsv1.NamespaceScoped | ||
} else { | ||
schema.VendorExtensible.AddExtension(common.ScopeExtensionKey, apiextensionsv1.ClusterScoped) | ||
if schema.VendorExtensible.Extensions == nil { | ||
schema.VendorExtensible.Extensions = map[string]any{} | ||
} | ||
schema.VendorExtensible.Extensions[common.ScopeExtensionKey] = apiextensionsv1.ClusterScoped |
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 was this changed, was therer a deprecation somewhere? The old code looked cleaner
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.
Overlooked this comment, now I noticed it, returned AddExtension method.
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.
A few questions:
-
How does it verify, that none of the "deep" nesting is provided, as we wanted it to be limited to one level deep?
-
How is prevented for watches, as with the field resolver we have the ability to execute this query on everything.
-
How would you solve the multiple kinds for different group errors? E.g. imagine a resource that has a
ThingRef
butThing
is provied via groupA
and groupB
- how would you know which would be the right kind to use for the relation?
if crd == nil { | ||
return b | ||
} | ||
|
||
schema, ok := b.schemas[getOpenAPISchemaKey(*gvk)] | ||
if !ok { | ||
gkv, err := getCRDGroupVersionKind(crd.Spec) |
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.
was it intentional, that the short path with the categories got removed?
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 did this change having in mind different versions of CRD.
Before Categories were applied to the "preferred" version of a CRD
After Categories are applied to ALL versions of a CRD
It is not directly related to the relations feature, but it is needed for consistency.
Let me know if it makes sense for you.
listener/pkg/apischema/builder.go
Outdated
if resourceSchema.VendorExtensible.Extensions == nil { | ||
resourceSchema.VendorExtensible.Extensions = map[string]any{} | ||
} | ||
resourceSchema.VendorExtensible.Extensions[common.CategoriesExtensionKey] = apiResource.Categories |
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.
Same question as above
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 touched this code as well for the same reason, but behavior remains the same compared to main branch - the change is in refactoring, less parsing and early returns.
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 was asking more about why the easily readale AddExtension
was removed with this custom logic
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.
That was not necessary, returned AddExtension methods instead of manual manipulation.
listener/pkg/apischema/builder.go
Outdated
key := strings.ToLower(gvk.Kind) | ||
b.kindRegistry[key] = append(b.kindRegistry[key], resourceInfo) |
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.
What happens if we have the same kind provided by different groups? We have a bug here. In go map keys can be arbitrary, so it would be more benefitial to use the GroupVersionKind
struct as an index here, rather than only the kind, the circumvent future name collisions with the kind.
Especially in the platform-mesh usecase this might be very common, that the same kind is provided form x different apiGroups
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.
you are right - we may have silent overrides here, fixing it.
Fixed
// Two schemas with same Kind different groups/versions - first should win | ||
first := schemaWithGVK("a.example", "v1", "Thing") | ||
second := schemaWithGVK("b.example", "v1", "Thing") | ||
b.SetSchemas(map[string]*spec.Schema{ | ||
"a.example.v1.Thing": first, | ||
"b.example.v1.Thing": second, | ||
"c.other.v1.Other": schemaWithGVK("c.other", "v1", "Other"), | ||
}) |
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 test is basically testing the exact wrong thing that I mentioned in the other comment - this would in my view be a bug, not intended 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.
I change a logic a bit - now server preferred resources has priority first, if no, we put in the first place resources from core group, then by version stability (v1 > v1beta1 > v1alpha1).
Logic is preserved in findBestResourceForKind.
And I added a test that checks if server preferred resources comes first.
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
On-behalf-of: @SAP [email protected] Signed-off-by: Artem Shcherbatiuk <[email protected]>
Resolves #254
Features
Testing
Tested 15.08 at
44afd8b
commit in local-setupDemo