-
Notifications
You must be signed in to change notification settings - Fork 236
Switch from containers/image to go-containerregistry #229
Conversation
@nkubala Out of curiosity: What was the reasoning behind writing the |
f02546a
to
09fa779
Compare
@vrothberg Google has had https://github.com/google/containerregistry, a registry library written in Python, for over a year now. Since most of our tools that we're working on these days are written in Go, we decided to port the library over. containers/image is definitely nice, but maintaining our own library gives us more flexibility and control over design decisions. |
ae34bcf
to
41df6b0
Compare
cmd/root.go
Outdated
}, nil | ||
func getImageForName(imageName string) (pkgutil.Image, error) { | ||
if !pkgutil.IsTar(imageName) && !pkgutil.HasTag(imageName) { | ||
imageName = imageName + pkgutil.LatestTag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have to add latest? WeakValidation should add that below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah you're right, removed
cmd/root.go
Outdated
if err != nil { | ||
return pkgutil.Image{}, err | ||
} | ||
// logrus.Infof("retrieving remote image: %v", ref) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uncomment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
c := configFile.Config | ||
return []string{ | ||
fmt.Sprintf("Hostname: %s", c.Hostname), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these need newlines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they don't, this is a list of strings that gets thrown into a generic result struct and outputted separately
differs/metadata_diff.go
Outdated
fmt.Sprintf("ArgsEscaped: %t", c.ArgsEscaped), | ||
fmt.Sprintf("Image: %s", c.Image), | ||
fmt.Sprintf("Volumes: %v", pkgutil.SortMap(c.Volumes)), | ||
// fmt.Sprintf("Workdir: %s", c.Workdir), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uncomment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah thanks, fixed workdir and removed stoptimeout since it's not part of the returned config
@nkubala nit: We've had it for about 4 years, but most of that internally :) |
This removes the dependency on containers/image in favor of our new go-containerregistry library.
Filesystem diffs are now done by extracting the flattened fs from the image.
Caching has been temporarily removed while I design a new solution, since the layer caching will no longer work. We can probably just naively cache the fs tarball for now.
The RPM differ needed to be rewritten to load the image into the docker daemon before running commands in a container. google/go-containerregistry#114 needs to be merged before this will compile, but should work correctly once we update the deps to pick up that commit.