fix ctr image export not found error#2622
Conversation
|
Thanks, but I think we should also support |
|
And please sign the commit with your full name |
|
@AkihiroSuda ok, thank you, if use |
|
no need to download, it should just fail when blob is not locally available |
kadisi
force-pushed
the
export_bug
branch
2 times, most recently
from
d9cad03 to
1af6379
Compare
|
requires rebase after export functions moved |
kadisi
force-pushed
the
export_bug
branch
2 times, most recently
from
093ada2 to
8c5316b
Compare
Codecov Report
@@ Coverage Diff @@
## master #2622 +/- ##
==========================================
- Coverage 47.5% 43.89% -3.61%
==========================================
Files 91 101 +10
Lines 8460 10800 +2340
==========================================
+ Hits 4019 4741 +722
- Misses 3717 5328 +1611
- Partials 724 731 +7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Do we want to keep Export() decoupled from oci.V1Exporter?
There was a problem hiding this comment.
I don't think we need to. I have been thinking about your other comment related to the importer and it might just make sense to unify them. The only real formats right now are OCI and Docker v1.1+, and we don't really expect those to change in a significant way to justify having a complex set of interfaces.
There was a problem hiding this comment.
Can you remove this change from this PR since it is unrelated
There was a problem hiding this comment.
Just use platforms.DefaultSpec()
There was a problem hiding this comment.
Can you also remove this import change, it is not related and causes an unnecessary conflict with the import PR
There was a problem hiding this comment.
I think it would be more consistent to just add the all platforms option here, then it can use that to create the V1 exporter. I kind of think we need to do some deeper cleaner, so we can focus on the refactoring and interfaces in one change and just focus on the platform feature and bug fix here.
There was a problem hiding this comment.
@dmcgowan i have changed exportOpts struct to type V1ExporterOpt func(c *V1Exporter) error . and use ResolveV1ExportOpt function to create the V1Exporter . if i add AllPlatforms feild to exportOpts struct, i must use exportOpts to create V1Exporter. Suppose if we need to add new fields to V1Exporter struct in the future, we also need add same field to exportOpts struct. Is this more cumbersome?
|
Sorry we just merged the import re-write and there is a conflict/requires rebase again |
|
ping ^^ @kadisi |
1 similar comment
|
ping ^^ @kadisi |
kadisi
force-pushed
the
export_bug
branch
2 times, most recently
from
53b9cbc to
2002481
Compare
|
@fuweid PTAL? |
Signed-off-by: Jie Zhang <[email protected]>
|
@dmcgowan want to take a final look at this? I think all feedback is handled |
|
ping @dmcgowan |
fuweid
left a comment
fuweid
left a comment
There was a problem hiding this comment.
The comment has more space and the platforms.DefaultSpec() is better than platforms.Parse because we don't need to do marshal/unmarshal things here.
| AllPlatforms bool | ||
| } | ||
|
|
||
| // V1ExporterOpt allows to set additional options to a newly V1Exporter |
|
Going to merge based on 2 LGTMs; will create a PR to handle the 2 minor cleanup suggestions |
6 participants
Signed-off-by: Jie Zhang [email protected]
fix #2590
may be need some test case