Reduce variable count #453
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
m1kola
force-pushed
the
reduce_variable_count
branch
from
bd1dd4e
to
f81686a
Compare
After this commit we will be creating one bundle per package. Previously we we creating one variable per bundle occurance in a channel which is currently unnecessary. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
m1kola
force-pushed
the
reduce_variable_count
branch
from
f81686a
to
497d929
Compare
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #453 +/- ##
==========================================
- Coverage 83.86% 83.80% -0.06%
==========================================
Files 23 23
Lines 849 846 -3
==========================================
- Hits 712 709 -3
Misses 94 94
Partials 43 43
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| if _, ok := visited[id]; ok { | ||
| continue | ||
| } | ||
| visited[id] = struct{}{} |
There was a problem hiding this comment.
Can we use sets.New[deppy.Identifier]() for clearer semantics on visited and added?
There was a problem hiding this comment.
AFAIK Go doesn't have sets in standard library and as a result using maps for sets is pretty common.
But we can do that. However I do not see any sets package in our codebase. What package is it?
Edit: is it k8s.io/apimachinery/pkg/util/sets?
There was a problem hiding this comment.
Anyway I think it should not be in this PR. Could you please create an issue or PR for this suggesting a pacakge to use?
There was a problem hiding this comment.
The k8s API machinery sets package is the one - we use it in basically every other package. Feel free to create the issue if you don't think you can do it here.
There was a problem hiding this comment.
I'm a fan of small PRs serve one porpuse. Don't want to mix different efforts into this PR.
Created #454 for this.
There was a problem hiding this comment.
These changes seem reasonable and look good to me, nice job on the variable reduction!
I'm approving, but would prefer waiting to merge on a second approval from someone who has better insight into the variable(sources) and resolution process as a whole to verify this doesn't have any cascading impacts.
| func NewBundleVariable(id deppy.Identifier, bundle *catalogmetadata.Bundle, dependencies []*catalogmetadata.Bundle) *BundleVariable { | ||
| var dependencyIDs []deppy.Identifier | ||
| func NewBundleVariable(bundle *catalogmetadata.Bundle, dependencies []*catalogmetadata.Bundle) *BundleVariable { | ||
| dependencyIDs := make([]deppy.Identifier, 0, len(dependencies)) | ||
| for _, bundle := range dependencies { |
There was a problem hiding this comment.
Nit: unrelated to this PR, but we can we change the loop variable to be called dependency instead of bundle so that it doesn't shadow the bundle variable passed in as a parameter?
There was a problem hiding this comment.
I'll submit a follow up PR straight after merging this one.
|
I experimented with t8c operator from https://operatorhub.io/: when installing without specifying version it currently creates 87 variables. With changes in this PR it creates 21 variables. I haven't mesured the preformance (with a naked eye it is unnoticable), but at least it will give a shorter error message in case of resolution failure. |
Description
After this commit we will be creating one bundle per package. Previously we we creating one variable per bundle occurance in a channel which is currently unnecessary.
Closes #414
Reviewer Checklist