Resolve bundle digests when DepV2 is enabled #3071
Conversation
Signed-off-by: Kim Christensen <kimworking@gmail.com>
kichristensen
requested review from
schristoff,
sgettys,
bdegeeter and
troy0820
as code owners
| func (p *Porter) resolveDependencyDigest(ctx context.Context, e *yaml.Editor, opts cnabtooci.RegistryOptions) error { | ||
| // find all referenced dependencies that does not have digest specified | ||
| // get the digest for all of them and update the manifest with the digest | ||
| return e.WalkNodes(ctx, "dependencies.requires.*", func(ctx context.Context, nc *yqlib.NodeContext) error { |
There was a problem hiding this comment.
My only concern is
- Should we resolve dependencies that aren't required (I feel yes?) - and:
- Are there any concerns with this and "shared dependencies", where the dependency is already installed and running (I think this should be okay)
There was a problem hiding this comment.
- I'm not sure I fully understand what you mean with "dependencies not required"
- I think it should be okay too, but I will create a test to verify it. It makes sense long term anyway
There was a problem hiding this comment.
@schristoff Number 2 Should be covered by the existing integration for shared dependencies,
Do you agree?
| return e.WriteFile(build.LOCAL_MANIFEST) | ||
| } | ||
|
|
||
| func (p *Porter) resolveDependencyDigest(ctx context.Context, e *yaml.Editor, opts cnabtooci.RegistryOptions) error { |
There was a problem hiding this comment.
Could this be reworked to leverage existing dependency resolution logic? - moving this into "validate" logic may help us reduce LOC and improve efficiency :)
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if p.IsFeatureEnabled(experimental.FlagDependenciesV2) { |
There was a problem hiding this comment.
Same as below (?) - I would love to see this moved into here - we can use this too :)
There was a problem hiding this comment.
Thank you for working on this. I've taken a stab at it before but would get sidetracked. I would love to see it using the validate() logic, which makes this a little straightforward but long term could improve efficiency and be more "predictable" in placement.
@schristoff Are you suggesting to use |
What does this change
When Dependencies V2 is enable bundle dependencies with a default bundle reference will be resolved to digests.
What issue does it fix
Closes #2676
Notes for the reviewer
Checklist