Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: globalEnvs for KO_DOCKER_REPO, annotations and labels for the image config based on build ids #632

Closed
wants to merge 1 commit into from
Closed

feat: globalEnvs for KO_DOCKER_REPO, annotations and labels for the image config based on build ids #632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkg/build/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,9 @@ type Config struct {
// ModTimestamp string `yaml:"mod_timestamp,omitempty"`
// GoBinary string `yaml:",omitempty"`
}

type ImageConfig struct {
ID string `yaml:",omitempty"`
Annotations map[string]string `yaml:",omitempty"`
Labels map[string]string `yaml:",omitempty"`
Comment on lines +102 to +103
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these just go in Config alongside Env etc.? It seems like a meaningless separation.

The only problem with that is if goreleaser adds a Labels or Annotations field to its config, we'd have a different meaning. I'm starting to really regret copying goreleaser's config so closely. 😢

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, we should separate them from a build because these are related to the image we provide at the end of that build. So, it might cause a bit more confusion if we put them into the Config section. In the current design, users should match the id fields of the builds and images sections to be able to define labels and annotations per image related to that build, WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, I was confusing build.env (what envs to set when we go build) vs image.env (what envs to set in the resulting image).

In that case, yeah, I agree we should have a separate image config struct with labels/annotations.

We should also use this to fix #633, with a top-level defaultImageConfig: { } option, that gets overridden by more specific images: and both get overridden if there's a conflicting --image-label specified.

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, we should apply some kind of prioritization between them, that makes a lot of sense 🙋🏻‍♂️

Here are the prioritization rules:

  1. The most important one is --image-label, if there is a conflict between others, we should use the defined one by the --image-label flag.
  2. The second important one is the images section within the .ko.yaml, so, if we specify the more specific one inside of that section, we should override the defaultImageConfig.
  3. The defaultImageConfig has the lowest priority among others. We should use the values within the defaultImageConfig unless others (images:, --image-label) do not specify any values at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be okay @imjasonh 🙋🏻‍♂️

}
28 changes: 28 additions & 0 deletions pkg/build/gobuild.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ type gobuild struct {
trimpath bool
preserveMediaType bool
buildConfigs map[string]Config
imageConfigs map[string]*ImageConfig
defaultImageConfig *ImageConfig
platformMatcher *platformMatcher
dir string
labels map[string]string
Expand All @@ -102,6 +104,8 @@ type gobuildOpener struct {
trimpath bool
preserveMediaType bool
buildConfigs map[string]Config
imageConfigs map[string]*ImageConfig
defaultImageConfig *ImageConfig
platforms []string
labels map[string]string
dir string
Expand Down Expand Up @@ -130,6 +134,8 @@ func (gbo *gobuildOpener) Open() (Interface, error) {
trimpath: gbo.trimpath,
preserveMediaType: gbo.preserveMediaType,
buildConfigs: gbo.buildConfigs,
imageConfigs: gbo.imageConfigs,
defaultImageConfig: gbo.defaultImageConfig,
labels: gbo.labels,
dir: gbo.dir,
platformMatcher: matcher,
Expand Down Expand Up @@ -803,6 +809,26 @@ func (g *gobuild) buildOne(ctx context.Context, refStr string, base v1.Image, pl
if cfg.Config.Labels == nil {
cfg.Config.Labels = map[string]string{}
}

annotations := map[string]string{}
// initialize default image labels and annotations
if g.defaultImageConfig != nil {
for k, v := range g.defaultImageConfig.Labels {
cfg.Config.Labels[k] = v
}
annotations = g.defaultImageConfig.Annotations
}

// override the defaultImageConfig.labels
if ic := g.imageConfigs[ref.Path()]; ic != nil {
for k, v := range ic.Labels {
cfg.Config.Labels[k] = v
}
for k, v := range ic.Annotations {
annotations[k] = v
}
}

for k, v := range g.labels {
cfg.Config.Labels[k] = v
}
Expand All @@ -817,6 +843,8 @@ func (g *gobuild) buildOne(ctx context.Context, refStr string, base v1.Image, pl
return nil, err
}

image = mutate.Annotations(image, annotations).(v1.Image)

si := signed.Image(image)

if g.sbom != nil {
Expand Down
34 changes: 34 additions & 0 deletions pkg/build/gobuild_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,9 +727,23 @@ func TestGoBuild(t *testing.T) {
WithCreationTime(creationTime),
WithBaseImages(func(context.Context, string) (name.Reference, Result, error) { return baseRef, base, nil }),
withBuilder(writeTempFile),
WithConfig(map[string]Config{
importpath: {ID: "ko"},
}),
WithImageConfig(map[string]*ImageConfig{
importpath: {ID: "ko", Labels: map[string]string{
"fooo": "baaar",
}},
}),
withSBOMber(fauxSBOM),
WithLabel("foo", "bar"),
WithLabel("hello", "world"),
WithDefaultImageConfig(&ImageConfig{
Labels: map[string]string{
"baz": "qux",
}, Annotations: map[string]string{
"qux": "quux",
}}),
)
if err != nil {
t.Fatalf("NewGo() = %v", err)
Expand Down Expand Up @@ -777,12 +791,32 @@ func TestGoBuild(t *testing.T) {
want := map[string]string{
"foo": "bar",
"hello": "world",
"baz": "qux",
}
got := cfg.Config.Labels
if d := cmp.Diff(got, want); d != "" {
t.Fatalf("Labels diff (-got,+want): %s", d)
}
})

t.Run("check annotations", func(t *testing.T) {
m, err := img.Manifest()
if err != nil {
t.Fatalf("Manifest() = %v", err)
}

want := map[string]string{
"qux": "quux",
}
got := m.Annotations

if d := cmp.Diff(got, want, cmp.FilterPath(func(p cmp.Path) bool {
return p.GoString() == "{map[string]string}[\"org.opencontainers.image.base.digest\"]" ||
p.GoString() == "{map[string]string}[\"org.opencontainers.image.base.name\"]"
}, cmp.Ignore())); d != "" {
t.Fatalf("Annotations diff (-got,+want): %s", d)
}
})
}

func TestGoBuildWithKOCACHE(t *testing.T) {
Expand Down
2 changes: 2 additions & 0 deletions pkg/build/gobuilds_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ func Test_gobuilds(t *testing.T) {
description string
workingDirectory string
buildConfigs map[string]Config
imageConfigs map[string]*ImageConfig
opts []Option
nilDefaultBuilder bool // set to true if you want to test build config and don't want the test to fall back to the default builder
importpath string
Expand Down Expand Up @@ -123,6 +124,7 @@ func Test_gobuilds(t *testing.T) {
if result == nil {
t.Fatalf("gobuilds.Build(%s): expected non-nil result", qualifiedImportpath)
}

})
}
}
Expand Down
14 changes: 14 additions & 0 deletions pkg/build/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,20 @@ func WithConfig(buildConfigs map[string]Config) Option {
}
}

func WithImageConfig(imageConfigs map[string]*ImageConfig) Option {
return func(gbo *gobuildOpener) error {
gbo.imageConfigs = imageConfigs
return nil
}
}

func WithDefaultImageConfig(defaultImageConfig *ImageConfig) Option {
return func(gbo *gobuildOpener) error {
gbo.defaultImageConfig = defaultImageConfig
return nil
}
}

// WithPlatforms is a functional option for building certain platforms for
// multi-platform base images. To build everything from the base, use "all",
// otherwise use a list of platform specs, i.e.:
Expand Down
3 changes: 3 additions & 0 deletions pkg/commands/apply.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ func addApply(topLevel *cobra.Command) {
if err != nil {
return fmt.Errorf("error creating builder: %w", err)
}
if bo.DockerRepo != "" {
po.DockerRepo = bo.DockerRepo
}
publisher, err := makePublisher(po)
if err != nil {
return fmt.Errorf("error creating publisher: %w", err)
Expand Down
3 changes: 3 additions & 0 deletions pkg/commands/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ func addBuild(topLevel *cobra.Command) {
if err != nil {
return fmt.Errorf("error creating builder: %w", err)
}
if bo.DockerRepo != "" {
po.DockerRepo = bo.DockerRepo
}
publisher, err := makePublisher(po)
if err != nil {
return fmt.Errorf("error creating publisher: %w", err)
Expand Down
3 changes: 3 additions & 0 deletions pkg/commands/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ func addCreate(topLevel *cobra.Command) {
if err != nil {
return fmt.Errorf("error creating builder: %w", err)
}
if bo.DockerRepo != "" {
po.DockerRepo = bo.DockerRepo
}
publisher, err := makePublisher(po)
if err != nil {
return fmt.Errorf("error creating publisher: %w", err)
Expand Down
56 changes: 56 additions & 0 deletions pkg/commands/options/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,14 @@ type BuildOptions struct {

// If true, don't convert Docker-typed base images to OCI when building.
PreserveMediaType bool
// ImageConfigs stores the per-image image config from `.ko.yaml`.
ImageConfigs map[string]*build.ImageConfig `yaml:",omitempty"`

// DockerRepo stores the docker repo from `.ko.yaml`.
DockerRepo string `yaml:",omitempty"`

// DefaultImageConfig stores the default image config for all the images ko built from `.ko.yaml`.
DefaultImageConfig *build.ImageConfig `yaml:",omitempty"`
}

func AddBuildOptions(cmd *cobra.Command, bo *BuildOptions) {
Expand Down Expand Up @@ -154,9 +162,57 @@ func (bo *BuildOptions) LoadConfig() error {
bo.BuildConfigs = buildConfigs
}

if len(bo.ImageConfigs) == 0 {
var imageConfigs []build.ImageConfig
if err := v.UnmarshalKey("images", &imageConfigs); err != nil {
return fmt.Errorf("configuration section 'images' cannot be parsed")
}
imageConfigsPerImage, err := createImageConfigMap(imageConfigs, bo.BuildConfigs)
if err != nil {
return fmt.Errorf("could not create image config map: %w", err)
}
bo.ImageConfigs = imageConfigsPerImage
}

if bo.DockerRepo == "" {
bo.DockerRepo = v.GetString("dockerRepo")
}

if bo.DefaultImageConfig == nil {
var defaultImageConfig build.ImageConfig
if err := v.UnmarshalKey("defaultImageConfig", &defaultImageConfig); err != nil {
return fmt.Errorf("configuration section 'defaultImageConfig' cannot be parsed")
}
bo.DefaultImageConfig = &defaultImageConfig
}

return nil
}

func createImageConfigMap(imageConfigs []build.ImageConfig, buildConfigs map[string]build.Config) (map[string]*build.ImageConfig, error) {
imageConfigsPerImage := make(map[string]*build.ImageConfig)

for ip, bc := range buildConfigs {
for i, config := range imageConfigs {
if config.ID == "" {
config.ID = fmt.Sprintf("#%d", i)
}

if config.Annotations == nil {
config.Annotations = map[string]string{}
}
if config.Labels == nil {
config.Labels = map[string]string{}
}

if bc.ID == config.ID {
imageConfigsPerImage[ip] = &config
}
}
}
return imageConfigsPerImage, nil
}

func createBuildConfigMap(workingDirectory string, configs []build.Config) (map[string]build.Config, error) {
buildConfigsByImportPath := make(map[string]build.Config)
for i, config := range configs {
Expand Down
3 changes: 3 additions & 0 deletions pkg/commands/resolve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ func addResolve(topLevel *cobra.Command) {
if err != nil {
return fmt.Errorf("error creating builder: %w", err)
}
if bo.DockerRepo != "" {
po.DockerRepo = bo.DockerRepo
}
Comment on lines +69 to +71
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bleh, this also doesn't feel right. Sorry to keep moving things around on you, I'm struggling to find a good way to express this without having multiple copies of this floating around.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about creating an overrider function to set and pass some variables from builder to publisher struct?

makeOverrider(bo, po)

I'm not sure if overrider is the right word but used it as a placeholder.

publisher, err := makePublisher(po)
if err != nil {
return fmt.Errorf("error creating publisher: %w", err)
Expand Down
8 changes: 8 additions & 0 deletions pkg/commands/resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,14 @@ func gobuildOptions(bo *options.BuildOptions) ([]build.Option, error) {
opts = append(opts, build.WithConfig(bo.BuildConfigs))
}

if bo.ImageConfigs != nil {
opts = append(opts, build.WithImageConfig(bo.ImageConfigs))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if WithImageConfig just handled nil? Then we could opts = append(opts, build.WithImageConfig(bo.ImageConfigs)) no matter what. (same with defaultImageConfig)

}

if bo.DefaultImageConfig != nil {
opts = append(opts, build.WithDefaultImageConfig(bo.DefaultImageConfig))
}

return opts, nil
}

Expand Down
3 changes: 3 additions & 0 deletions pkg/commands/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ func addRun(topLevel *cobra.Command) {
if err != nil {
return fmt.Errorf("error creating builder: %w", err)
}
if bo.DockerRepo != "" {
po.DockerRepo = bo.DockerRepo
}
publisher, err := makePublisher(po)
if err != nil {
return fmt.Errorf("error creating publisher: %w", err)
Expand Down