fix(indexing): respect strict mode #523
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: njhale The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
Codecov Report
@@ Coverage Diff @@
## master #523 +/- ##
==========================================
- Coverage 49.42% 49.41% -0.02%
==========================================
Files 90 90
Lines 6286 6284 -2
==========================================
- Hits 3107 3105 -2
Misses 2485 2485
Partials 694 694
Continue to review full report at Codecov.
|
|
/retest |
| @@ -90,7 +90,7 @@ func (r *ReplacesInputStream) canAdd(bundle *Bundle, packageGraph *Package) erro | |||
|
|
|||
| if replaces != "" && !packageGraph.HasCsv(replaces) { | |||
| // We can't add this until a replacement exists | |||
| return fmt.Errorf("Invalid bundle %s, bundle specifies a non-existent replacement %s", bundle.Name, replaces) | |||
| return fmt.Errorf("Invalid bundle %s, replaces nonexistent bundle %s", bundle.Name, replaces) | |||
There was a problem hiding this comment.
This feels more like a personal choice and unrelated to the fix the PR is trying to address.
There was a problem hiding this comment.
sure, but the existing wording doesn't make sense.
There was a problem hiding this comment.
If we feel that was we can probably open another housekeeping PR that's titled fix log message. I'm just not sure if changing log message goes with respect strict mode
There was a problem hiding this comment.
I feel strongly that we shouldn't tax CI for trivial changes. I think the k8s docs also suggest that trival changes should go into larger PRs for that project.
pkg/registry/input_stream.go
Outdated
| @@ -147,7 +147,6 @@ func (r *ReplacesInputStream) Next() (*ImageInput, error) { | |||
|
|
|||
| // No viable bundle found in the package, can't parse it any further | |||
| if len(packageErrs) > 0 { | |||
There was a problem hiding this comment.
Do we need this check anymore? Cant we just append the errors regardless?
There was a problem hiding this comment.
Yup, looks like that would work for the edge cases: https://play.golang.org/p/_SNa9QLWEhQ
| @@ -196,7 +196,6 @@ func (i *DirectoryPopulator) loadManifests(imagesToAdd []*ImageInput, imagesToRe | |||
| errs = append(errs, err) | |||
| break | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
nit: Seems like some noise in a file not being touched by this PR.
There was a problem hiding this comment.
I did this on purpose, didn't like the extra space; I'm pretty sure I introduced this in the previous patch too.
There was a problem hiding this comment.
I'd put forth the same argument as this comment, but if you feel strongly about it anyway it's a small PR so I guess it's not worth delaying this PR because of this.
Keep packages in the input stream regardless of errors attempting to add bundles of that package to an index. This prevents invalid bundles from being elided before all valid bundles have been exhausted, and fixes a regression that effectively broke strict (non-permissive) mode.
njhale
force-pushed
the
fix-permissive
branch
from
19ab591 to
09239b7
Compare
| delete(r.packages, pkg) | ||
| errs = append(errs, packageErrs...) | ||
| } | ||
| // No viable bundle found in the package, can't parse it any further, so return any errors |
There was a problem hiding this comment.
nit: Is this comment still relevant?
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Keep packages in the input stream regardless of errors attempting to add
bundles of that package to an index. This prevents invalid bundles from
being elided before all valid bundles have been exhausted, and fixes a
regression that effectively broke strict (non-permissive) mode.
Supersedes #522