Skip to content
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.

Sign up for GitHub

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

Removes GVKRequired struct #400

Merged
merged 1 commit into from
Sep 8, 2023
Merged

Removes GVKRequired struct #400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 3 additions & 13 deletions internal/resolution/entities/bundle_entity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,23 +47,13 @@ func (g GVK) String() string {
return fmt.Sprintf(`group:"%s" version:"%s" kind:"%s"`, g.Group, g.Version, g.Kind)
}

type GVKRequired property.GVKRequired
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have the type GVK property.GVK type alias above? Why do we need one of these two aliases but not both?

Copy link
Member Author

Choose a reason for hiding this comment

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

@stevekuznetsov I beleive this is only for use to define String() method on it.

I see no reason for GVKRequired to exist as both are esentially the same struct\schema.

Copy link
Member Author

@m1kola m1kola Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Another guess: probably we re-define structs so we don't spread property.GVK across controller-runtime codebase.

Copy link
Member

Choose a reason for hiding this comment

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

a) if we need aliases here, we need all of them
b) if there's a semantic difference between GVK and GVKRequired, we need to keep that difference here
c) if there's not a difference, let's go to "github.com/operator-framework/operator-registry/alpha/property" and remove GVKRequired.

so we don't spred property.GVK across controller-runtime codebase.

What does this mean? Why would we not want to do "spread"? What are the implications?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha! I didn't even notice GVKRequired was an alias for property.GVKRequired.

if there's a semantic difference between GVK and GVKRequired, we need to keep that difference here

There is semantic difference between properties (they have different type), but not between structs: they both represent GVK. See:

What does this mean? Why would we not want to do "spread"? What are the implications?

Like probably there was an idea to redefine struct in operator controller to have less dependency on operator-registry? I don't know, I'm just guessing what the original author meant 🤷‍♂️.

Copy link
Member

Choose a reason for hiding this comment

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

There is semantic difference between properties (they have different type), but not between structs: they both represent GVK.

If there is a semantic difference, I don't know that we can remove the use of one. Users are expected to understand the implication of each.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are not removing olm.gvk.required property type. User will still be using it. I'm only proposing to re-use a struct into which we unmarshal values of properties of both types olm.gvk and olm.gvk.required. GVKRequired is being converted into GVK anyway.

But I agree with you - this should be done on operator-registry side first. When creating this PR I didn't notice that that operator-registry also has two different structs for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, I see. It looks like the only purpose of separate GVKRequired is to enable operator-registry to get its type using reflection and map it to a property type.


func (g GVKRequired) String() string {
return fmt.Sprintf(`group:"%s" version:"%s" kind:"%s"`, g.Group, g.Version, g.Kind)
}

func (g GVKRequired) AsGVK() GVK {
return GVK(g)
}

type BundleEntity struct {
*input.Entity

// these properties are lazy loaded as they are requested
bundlePackage *property.Package
providedGVKs []GVK
requiredGVKs []GVKRequired
requiredGVKs []GVK
requiredPackages []PackageRequired
channel *property.Channel
channelEntry *ChannelEntry
Expand Down Expand Up @@ -101,7 +91,7 @@ func (b *BundleEntity) ProvidedGVKs() ([]GVK, error) {
return b.providedGVKs, nil
}

func (b *BundleEntity) RequiredGVKs() ([]GVKRequired, error) {
func (b *BundleEntity) RequiredGVKs() ([]GVK, error) {
if err := b.loadRequiredGVKs(); err != nil {
return nil, err
}
Expand Down Expand Up @@ -214,7 +204,7 @@ func (b *BundleEntity) loadRequiredGVKs() error {
b.mu.Lock()
defer b.mu.Unlock()
if b.requiredGVKs == nil {
requiredGVKs, err := loadFromEntity[[]GVKRequired](b.Entity, property.TypeGVKRequired, optional)
requiredGVKs, err := loadFromEntity[[]GVK](b.Entity, property.TypeGVKRequired, optional)
if err != nil {
return fmt.Errorf("error determining bundle required gvks for entity '%s': %w", b.ID, err)
}
Expand Down
23 changes: 1 addition & 22 deletions internal/resolution/entities/bundle_entity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ var _ = Describe("BundleEntity", func() {
bundleEntity := olmentity.NewBundleEntity(entity)
requiredGvks, err := bundleEntity.RequiredGVKs()
Expect(err).ToNot(HaveOccurred())
Expect(requiredGvks).To(Equal([]olmentity.GVKRequired{
Expect(requiredGvks).To(Equal([]olmentity.GVK{
{Group: "foo.io", Kind: "Foo", Version: "v1"},
{Group: "bar.io", Kind: "Bar", Version: "v1alpha1"},
}))
Expand Down Expand Up @@ -326,27 +326,6 @@ var _ = Describe("BundleEntity", func() {
})
})

// Increase test coverage
Describe("GVKRequired properties", func() {
It("should return the GVKRequired properties", func() {
gvk := olmentity.GVKRequired{
Group: "foo.io",
Kind: "Foo",
Version: "v1",
}
Expect(gvk.AsGVK().Version).To(Equal("v1"))
Expect(gvk.AsGVK().Group).To(Equal("foo.io"))
Expect(gvk.AsGVK().Kind).To(Equal("Foo"))
})
It("should return the GVKRequired properties as a string", func() {
gvk := olmentity.GVKRequired{
Group: "foo.io",
Kind: "Foo",
Version: "v1",
}
Expect(gvk.String()).To(Equal(`group:"foo.io" version:"v1" kind:"Foo"`))
})
})
Describe("GVK properties", func() {
It("should return the gvk properties", func() {
gvk := olmentity.GVK{
Expand Down
2 changes: 1 addition & 1 deletion internal/resolution/variablesources/bundles_and_dependencies.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func (b *BundlesAndDepsVariableSource) getEntityDependencies(ctx context.Context
// todo(perdasilva): disambiguate between not found and actual errors
gvkDependencies, _ := bundleEntity.RequiredGVKs()
for i := 0; i < len(gvkDependencies); i++ {
providedGvk := gvkDependencies[i].AsGVK()
providedGvk := gvkDependencies[i]
gvkDependencyBundles, err := entitySource.Filter(ctx, predicates.ProvidesGVK(&providedGvk))
if err != nil {
return nil, err
Expand Down