[resolution pt.1] bundle entity #95
Conversation
perdasilva
force-pushed
the
bundle_entity
branch
from
149e01a to
fa0e642
Compare
Signed-off-by: perdasilva <perdasilva@redhat.com>
Signed-off-by: perdasilva <perdasilva@redhat.com>
Signed-off-by: perdasilva <perdasilva@redhat.com>
perdasilva
force-pushed
the
bundle_entity
branch
2 times, most recently
from
9017ba0 to
a55a00f
Compare
perdasilva
changed the title
perdasilva
changed the title
perdasilva
changed the title
| } | ||
|
|
||
| func (b *BundleEntity) loadPackage() error { | ||
| if b.bundlePackage == nil { |
There was a problem hiding this comment.
Is there no mutex lock required here unlike the other loadX funcs?
There was a problem hiding this comment.
Thank you!! Fixed
| if b.semVersion == nil { | ||
| semVer, err := semver.Parse(b.bundlePackage.Version) | ||
| if err != nil { | ||
| return fmt.Errorf("could not parse semver (%s) for entity '%s': %s", b.bundlePackage.Version, b.ID, err) |
There was a problem hiding this comment.
Nit: use %w when wrapping errors in fmt.Errorf().
Here and below.
There was a problem hiding this comment.
Good call! Fixed =D
There was a problem hiding this comment.
In case anyone is wondering why you should use %w when wrapping errors in fmt.Errorf(), checkout this blog.
| if errComp != 0 { | ||
| // the sign gets inverted because version is sorted | ||
| // from highest to lowest | ||
| return -1 * errComp |
There was a problem hiding this comment.
So, this will put all non-valid versions at the end?
There was a problem hiding this comment.
Does this case get tested?
There was a problem hiding this comment.
It does, but your comment compelled me to 1) make sure 2) improve it =D
| func (b *BundleEntity) loadProvidedGVKs() error { | ||
| b.mu.Lock() | ||
| defer b.mu.Unlock() | ||
| if b.providedGVKs == nil { |
There was a problem hiding this comment.
Is it possible for these values to become out-of-date? If so, if the value in the map was ever updated, this would continue to return the old value right?
There was a problem hiding this comment.
Great observation and question!!
This is read only and just serves to wrap around the Entity, which it doesn't expect to change during its lifetime.
There was a problem hiding this comment.
which it doesn't expect to change during its lifetime.
We may want to document any assumptions that we make. Do we prevent cache changes when deppy is solving?
There was a problem hiding this comment.
it == BundleEntity. The corresponding entity can change, but only on the source of truth (e.g. catalog source). But from the time you pull the entity down from source until resolution, it shouldn't change.
There was a problem hiding this comment.
Thanks for answering! Makes perfect sense to me.
perdasilva
force-pushed
the
bundle_entity
branch
from
a55a00f to
b4557d6
Compare
|
Getting failures now. |
Signed-off-by: perdasilva <perdasilva@redhat.com>
perdasilva
force-pushed
the
bundle_entity
branch
from
b4557d6 to
b0ecaf3
Compare
|
You will need to rebase your other PRs. |
| bundleEntity := olmentity.NewBundleEntity(entity) | ||
| name, err := bundleEntity.PackageName() | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
Non-blocking nit: should we add debug logs to these messages?
| }) | ||
|
|
||
| Describe("InChannel", func() { | ||
| It("should return true when the entity has the has version in the right range", func() { |
There was a problem hiding this comment.
Non-blocking nit, but something we should fix.
| It("should return true when the entity has the has version in the right range", func() { | |
| It("should return true when the entity comes from the specified channel", func() { |
4 participants
This PR is the first in a series of PRs that bring bundle resolution to the operator-controller.
This PR adds:
BundleEntitystruct that makes it easier to deal with the deppy entities we'll be processingPredicatesto search for particular entities based on the bundle propertiesSortfunctions to order the entities based on their bundle properties (Note: default channel is not yet handled properly)