rkt: Implement distribution points #3369
Conversation
| } | ||
|
|
||
| // NewACIArchive creates a new aci-archive distribution from the provided distribution uri | ||
| // string |
| return nil, fmt.Errorf("cannot parse URI: %q: %v", u.String(), err) | ||
| } | ||
| if c.Type != TypeACIArchive { | ||
| return nil, fmt.Errorf("wrong distribution type: %q", c.Type) |
| // This should be a valid URL | ||
| data, err := url.QueryUnescape(c.Data) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("wrong archive transport url %q: %v", c.Data, err) |
| } | ||
| aciu, err := url.Parse(data) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("wrong archive transport url %q: %v", c.Data, err) |
|
|
||
| str := u.String() | ||
|
|
||
| path := aciu.String() |
There was a problem hiding this comment.
superfluous newlines
|
|
||
| // SimpleDockerString returns a simplyfied docker cimd. This means removing | ||
| // the index url if it's the default docker registry (registry-1.docker.io), | ||
| // removing the default repo (library) when using the default docker registry |
There was a problem hiding this comment.
correct this doc, we are not returning a cimd
|
|
||
| // FullDockerCIMD return the docker cimd populated with all the default values | ||
| // docker strings like "busybox" or | ||
| // "registry-1.docker.io/library/busybox:latest" will become the same docker |
There was a problem hiding this comment.
correct this doc, we are not returning a cimd
| func DistFromImageString(is string) (dist.Distribution, error) { | ||
| u, err := url.Parse(is) | ||
| if err != nil { | ||
| errwrap.Wrap(fmt.Errorf("failed to parse image string %q", is), err) |
There was a problem hiding this comment.
failed to parse image string %q as url
| // FetchImage will take an image as either a path, a URL or a name | ||
| // string and import it into the store if found. If ascPath is not "", | ||
| // it must exist as a local file and will be used as the signature | ||
| // file for verification, unless verification is disabled. If | ||
| // f.WithDeps is true also image dependencies are fetched. | ||
| func (f *Fetcher) FetchImage(img string, ascPath string, imgType apps.AppImageType) (string, error) { | ||
| func (f *Fetcher) FetchImage(d dist.Distribution, str, ascPath string) (*types.Hash, error) { |
There was a problem hiding this comment.
the name str is too ambiguous
| h, err := types.NewHash(hash) | ||
| if err != nil { | ||
| // should never happen | ||
| log.PanicE("got an invalid hash, looks like it is corrupted", err) |
33685a8 to
d4542e5
Compare
| }, nil | ||
| } | ||
|
|
||
| // NewCIMDString creates a new cimd URL |
There was a problem hiding this comment.
NewCIMDString creates a new cimd URL string.
| // Distribution is the interface that represents a distribution point. | ||
| type Distribution interface { | ||
| // URI returns a copy of the Distribution CIMD URI. | ||
| URI() *url.URL |
There was a problem hiding this comment.
simply call this CIMD()?
175f3b6 to
97fa428
Compare
| @@ -218,9 +218,15 @@ func DistFromImageString(is string) (dist.Distribution, error) { | |||
| return nil, fmt.Errorf("docker distribution creation error: %v", err) | |||
| } | |||
| return dist, nil | |||
| default: | |||
| // Try to create a dist directly from the provided URI | |||
| case "cimd": | |||
There was a problem hiding this comment.
reference the cimd schme constant directly
519b311 to
3bd8afa
Compare
| @@ -79,12 +112,17 @@ func (f *Fetcher) getAsc(ascPath string) *asc { | |||
| // fetchImageDeps will recursively fetch all the image dependencies | |||
| func (f *Fetcher) fetchImageDeps(hash string) error { | |||
| imgsl := list.New() | |||
| seen := map[string]struct{}{} | |||
| seen := map[dist.Distribution]struct{}{} | |||
There was a problem hiding this comment.
this seems very dangerous to me as it is very easy to miss the equality invariant of the underlying dynamic type. I'd rather make this map[string]dist.Distrbution, where the key is the string representation of the cimd URL.
|
@lucab PTAL |
| // dependencies is correct since it may break noarch dependencies. Reference: | ||
| // https://github.com/coreos/rkt/pull/2317 | ||
| func (db *distBundle) setAppDefaults() error { | ||
| app := db.dist.(*dist.Appc 9E88 ).App() | ||
| if _, ok := app.Labels["arch"]; !ok { | ||
| app.Labels["arch"] = runtime.GOARCH |
There was a problem hiding this comment.
Note to self: this will need to be adjusted following appc/spec#668.
| import ( | ||
| "sort" | ||
|
|
||
| "github.com/appc/spec/schema/types" |
There was a problem hiding this comment.
If this is something appc-specific, should we somehow label this to make it more explicit (and perhaps move to appc/spec at some point in the future)?
There was a problem hiding this comment.
As discussed OOB: This is indeed appc-specific, and makes sense to move to appc/spec itself.
There was a problem hiding this comment.
Ack. I'm fine to have it here for the moment to avoid a tag-and-sync delay, just leave a TODO note at the top for the moment.
| return Get(u) | ||
| } | ||
|
|
||
| // from github.com/PuerkitoBio/purell |
There was a problem hiding this comment.
Please add the relevant license (BSD-3) to the header here, as well as the author. And this comment would better reference the exact file and version we took this from, for future-proof inspection.
There was a problem hiding this comment.
hmm, I'll simply import the library, it is small, and we will have a direct dependency. Since sortQuery modifies the given url in-place using purell.NormalizeURL will work.
| func DistFromImageString(is string) (dist.Distribution, error) { | ||
| u, err := url.Parse(is) | ||
| if err != nil { | ||
| errwrap.Wrap(fmt.Errorf("failed to parse image url %q", is), err) |
| for _, l := range ls { | ||
| out = append(out, fmt.Sprintf("%q:%q", l.Name, l.Value)) | ||
| } | ||
| return "[" + strings.Join(out, ", ") + "]" |
There was a problem hiding this comment.
This doesn't look too nice, and seems to used exactly once in the code for error-printing. Should we get rid of it?
There was a problem hiding this comment.
ack, the error format can refer directly to the %q fmt qualifier which will print i.e. [{"version" "1.0"} {"os" "linux"}] ..., so there is no need for this function.
c70c6e7 to
0382834
Compare
|
This LGTM, but CI looks very confused here. Perhaps @s-urbaniak could re-trigger it later. |
Currently glide errors out because some dependencies could not be found: [ERROR] Error scanning google.golang.org/grpc/stats: open /home/sur/.glide/cache/src/https-google.golang.org-grpc/stats: no such file or directory [ERROR] This error means the referenced package was not found. [ERROR] Missing file or directory errors usually occur when multiple packages [ERROR] share a common dependency and the first reference encountered by the scanner [ERROR] sets the version to one that does not contain a subpackage needed required [ERROR] by another package that uses the shared dependency. Try setting a [ERROR] version in your glide.yaml that works for all packages that share this [ERROR] dependency. [ERROR] Error scanning google.golang.org/grpc/tap: open /home/sur/.glide/cache/src/https-google.golang.org-grpc/tap: no such file or directory ... This fixes it by introducing them manually.
This removes the `-u` parameter suggestion for `glide get` since it is the default now.
This patch implements the distribution point concept with the three initial kind of distributions already used by rkt (appc, aci-archive, docker). It just makes the smallest required changes to the fetchers logic to make them work and pass the functional tests. This is the base for future fetchers (a future patch will refactor the fetcher logic to better fit the distribution concept) and store refactors.
Currently if the user specifies an app name like "app-name:v1.0" it won't be recognized because "app-name" is treated like an unknown URL scheme. This fixes it by assuming that unknown URL schemes imply appc images. Fixes rkt#3326
This is the implementation of the distribution concept proposed in #2953.
Supersedes #3101
This is work in progress to check the output of CI.