From 38b93fb2d2d63f1bdbfabc8c2c462f46b04ac2bf Mon Sep 17 00:00:00 2001 From: dekiel Date: Fri, 27 Sep 2024 11:36:59 +0200 Subject: [PATCH] Do not set base sha for pull request images. Set only tagger fields needed for image type. Fix validation regex for commit to align with expected date format. Return error from TagOption to signal problems with setting it. Checking if pr or sha strings passed to TagOption is empty. Return error when trying to set empty string. --- cmd/image-builder/main.go | 27 ++++++++++++++---------- configs/kaniko-build-config.yaml | 2 +- pkg/tags/options.go | 22 +++++++++++-------- pkg/tags/tag.go | 33 ++--------------------------- pkg/tags/tags.go | 36 +++++++++++--------------------- 5 files changed, 44 insertions(+), 76 deletions(-) diff --git a/cmd/image-builder/main.go b/cmd/image-builder/main.go index 8d1dabc934a5..0ae3a44a78e6 100644 --- a/cmd/image-builder/main.go +++ b/cmd/image-builder/main.go @@ -638,9 +638,16 @@ func (l *StrList) List() []string { } func getTags(pr, sha string, templates []tags.Tag) ([]tags.Tag, error) { + var taggerOptions []tags.TagOption + if len(pr) > 0 { + taggerOptions = append(taggerOptions, tags.PRNumber(pr)) + } + if len(sha) > 0 { + taggerOptions = append(taggerOptions, tags.CommitSHA(sha)) + } // build a tag from commit SHA - tagger, err := tags.NewTagger(templates, tags.CommitSHA(sha), tags.PRNumber(pr)) + tagger, err := tags.NewTagger(templates, taggerOptions...) if err != nil { return nil, fmt.Errorf("get tagger: %w", err) } @@ -910,17 +917,15 @@ func getEnvs(o options, dockerfilePath string) (map[string]string, error) { } func parseTags(o options) ([]tags.Tag, error) { - var pr string - sha := o.gitState.BaseCommitSHA - if o.gitState.isPullRequest { - pr = fmt.Sprint(o.gitState.PullRequestNumber) + var ( + pr string + sha string + ) + if !o.gitState.isPullRequest && o.gitState.BaseCommitSHA == "" { + sha = o.gitState.BaseCommitSHA } - - // TODO (dekiel): - // when running for pr we should enforce a sha to be empty because the base branch commit is not relevant for tags generated on pr. - // This variable should better be named to represent what sha it holds. - if sha == "" { - return nil, fmt.Errorf("sha still empty") + if o.gitState.isPullRequest && o.gitState.PullRequestNumber > 0 { + pr = fmt.Sprint(o.gitState.PullRequestNumber) } // read tags from base64 encoded string if provided diff --git a/configs/kaniko-build-config.yaml b/configs/kaniko-build-config.yaml index 0803f4e20b5f..3ff81eff1dfe 100644 --- a/configs/kaniko-build-config.yaml +++ b/configs/kaniko-build-config.yaml @@ -1,7 +1,7 @@ default-commit-tag: name: "default_tag" value: "v{{ .Date }}-{{ .ShortSHA }}" - validation: "^(v[0-9]+\\.[0-9]+\\.[0-9]+-\\d{8}-[0-9a-f]{7})$" + validation: "^(v[0-9]{8}-[0-9a-f]{8})$" default-pr-tag: name: "default_tag" value: "PR-{{ .PRNumber }}" diff --git a/pkg/tags/options.go b/pkg/tags/options.go index 57ced86ccaf6..fa8d39fa6762 100644 --- a/pkg/tags/options.go +++ b/pkg/tags/options.go @@ -4,28 +4,32 @@ import ( "fmt" ) -type TagOption func(o *Tagger) +type TagOption func(o *Tagger) error func DateFormat(format string) TagOption { - return func(t *Tagger) { + return func(t *Tagger) error { t.Date = t.Time.Format(format) + return nil } } func CommitSHA(sha string) TagOption { - return func(t *Tagger) { + return func(t *Tagger) error { + if len(sha) == 0 { + return fmt.Errorf("sha cannot be empty") + } t.CommitSHA = sha - // TODO (dekiel): This should be logged as a warning, not considered as an error - // if t.CommitSHA == "" { - // return nil, errors.New("variable CommitSHA is empty") - // } t.ShortSHA = fmt.Sprintf("%.8s", t.CommitSHA) + return nil } } func PRNumber(pr string) TagOption { - return func(t *Tagger) { + return func(t *Tagger) error { + if len(pr) == 0 { + return fmt.Errorf("pr number cannot be empty") + } t.PRNumber = pr - // TODO (dekiel): The empty string should be logged as a warning. + return nil } } diff --git a/pkg/tags/tag.go b/pkg/tags/tag.go index d0825b5d8a23..b391336b5d2d 100644 --- a/pkg/tags/tag.go +++ b/pkg/tags/tag.go @@ -4,8 +4,6 @@ import ( "fmt" "regexp" "strings" - - "gopkg.in/yaml.v3" ) // Tag store informations about single Tag @@ -13,7 +11,8 @@ type Tag struct { // Name which identifies single Tag Name string `yaml:"name" json:"name"` // Value of the tag or template of it - Value string `yaml:"value" json:"value"` + Value string `yaml:"value" json:"value"` + // Validation is a regex pattern to validate the tag value after it has been parsed Validation string `yaml:"validation" json:"validation"` } @@ -38,31 +37,3 @@ func NewTagFromString(val string) (Tag, error) { return t, nil } - -// UnmarshalYAML provides custom logic for unmarshalling tag into struct -// If not name is given it will be replaced by default_tag. -// It ensures that both use cases are supported -// TODO (dekiel): yaml config can provide a name always. -// -// This custom unmarshaller is not needed. -func (t *Tag) UnmarshalYAML(value *yaml.Node) error { - var tagTemplate string - - if err := value.Decode(&tagTemplate); err == nil { - t.Name = "default_tag" - t.Value = tagTemplate - return nil - } - - var tag map[string]string - - err := value.Decode(&tag) - if err != nil { - return err - } - - t.Name = tag["name"] - t.Value = tag["value"] - - return nil -} diff --git a/pkg/tags/tags.go b/pkg/tags/tags.go index 6e7fbd736044..284663776d8b 100644 --- a/pkg/tags/tags.go +++ b/pkg/tags/tags.go @@ -18,22 +18,17 @@ type Tagger struct { Date string } -// TODO (@Ressetkk): Evaluate if we really need to implement it in central way -// func (tg *Tagger) AddFlags(fs *flag.FlagSet) { -// fs.Var(&tg.tags, "tag", "Go-template based tag") -// } - func (tg *Tagger) Env(key string) string { return os.Getenv(key) } func (tg *Tagger) ParseTags() ([]Tag, error) { var parsed []Tag - for _, t := range tg.tags { - if len(t.Name) == 0 || len(t.Value) == 0 { - return nil, fmt.Errorf("tag name or value is empty, tag name: %s, tag value: %s", t.Name, t.Value) + for _, tag := range tg.tags { + if len(tag.Name) == 0 || len(tag.Value) == 0 { + return nil, fmt.Errorf("tag name or value is empty, tag name: %s, tag value: %s", tag.Name, tag.Value) } - tmpl, err := template.New("tag").Parse(t.Value) + tmpl, err := template.New("tag").Parse(tag.Value) if err != nil { return nil, err } @@ -42,10 +37,7 @@ func (tg *Tagger) ParseTags() ([]Tag, error) { if err != nil { return nil, err } - tag := Tag{ - Name: t.Name, - Value: buf.String(), - } + tag.Value = buf.String() err = tg.validateTag(tag) if err != nil { return nil, err @@ -57,15 +49,15 @@ func (tg *Tagger) ParseTags() ([]Tag, error) { } func (tg *Tagger) validateTag(tag Tag) error { - if tag.Name == "default_tag" && tag.Validation == "" { - return fmt.Errorf("default_tag validation is empty") + if tag.Name == "default_tag" && len(tag.Validation) == 0 { + return fmt.Errorf("default_tag validation is empty, tag: %s", tag.Value) } if tag.Validation != "" { // Verify PR default tag. Check if value starts with PR- and is followed by a number re := regexp.MustCompile(tag.Validation) match := re.FindAllString(tag.Value, -1) if match == nil { - return fmt.Errorf("default_tag validation failed") + return fmt.Errorf("tag validation failed, tag: %s, validation: %s", tag.Value, tag.Validation) } } return nil @@ -79,14 +71,10 @@ func NewTagger(tags []Tag, opts ...TagOption) (*Tagger, error) { Date: now.Format("20060102"), } for _, o := range opts { - o(&t) + err := o(&t) + if err != nil { + return nil, fmt.Errorf("error applying tag option: %w", err) + } } - // TODO (dekiel): this should be valideted outside of constructor. - // The tagger should be able to work for pr default tag or commit default tag. - // The different default tags require different values. - // if t.CommitSHA == "" { - // return nil, errors.New("variable CommitSHA is empty") - // } - // t.ShortSHA = fmt.Sprintf("%.8s", t.CommitSHA) return &t, nil }