Skip to content

Commit

Permalink
Refactor the way the Pipelines controllers receive flags.
Browse files Browse the repository at this point in the history
This change has a few parts, but the overarching goal is to make it easier for downstreams to repackage Tekton Pipelines across updates to the flags that are parsed and passed into the controllers.
1. Instead of defining `var`s for the flags, and then extracting the flags into the `pipeline.Images` struct, this change takes advantage of `flag.NewFooVar` to pass in the address of the variable to update.  This lets us have flag parsing process things directly into its final variable.
2. Wrap `Images` in `FlagOptions`, which is passed to controllers.  I'm sort of ambivalent about the nested struct vs. renaming Images, but opted for the former because so many files reference `pipeline.Images` and I wanted to reduce the scope of the change.
3. For flags only used in `main()` use a local flag definition (vs. `FlagOptions`).  It turns out the `*namespace` we were passing to the `NewController` methods was unused!
4. For globals we've turned into flags, use the `flag.NewFooVar` form to update the global directly.
5. Colocate the `flag` setup with the validation setup.  This allows us to get coverage of the flag processing code, and it turns out there were some bugs in the names of flags emitted from validation!

(cherry picked from commit d0a1053)
  • Loading branch information
mattmoor authored and tekton-robot committed Sep 29, 2021
1 parent cc7febe commit f8c2eea
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 11 deletions.
2 changes: 2 additions & 0 deletions cmd/controller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ func main() {
flag.BoolVar(&opts.ExperimentalDisableResolution, "experimental-disable-in-tree-resolution", false,
"Disable resolution of taskrun and pipelinerun refs by the taskrun and pipelinerun reconcilers.")

// This parses flags.
cfg := injection.ParseAndGetRESTConfigOrDie()

if err := opts.Images.Validate(); err != nil {
log.Fatal(err)
}
Expand Down
18 changes: 9 additions & 9 deletions pkg/apis/pipeline/images.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,15 @@ func (i Images) Validate() error {
for _, f := range []struct {
v, name string
}{
{i.EntrypointImage, "entrypoint"},
{i.NopImage, "nop"},
{i.GitImage, "git"},
{i.KubeconfigWriterImage, "kubeconfig-writer"},
{i.ShellImage, "shell"},
{i.ShellImageWin, "windows-shell"},
{i.GsutilImage, "gsutil"},
{i.PRImage, "pr"},
{i.ImageDigestExporterImage, "imagedigest-exporter"},
{i.EntrypointImage, "entrypoint-image"},
{i.NopImage, "nop-image"},
{i.GitImage, "git-image"},
{i.KubeconfigWriterImage, "kubeconfig-writer-image"},
{i.ShellImage, "shell-image"},
{i.ShellImageWin, "shell-image-win"},
{i.GsutilImage, "gsutil-image"},
{i.PRImage, "pr-image"},
{i.ImageDigestExporterImage, "imagedigest-exporter-image"},
} {
if f.v == "" {
unset = append(unset, f.name)
Expand Down
1 change: 0 additions & 1 deletion pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ import (
)

var (
namespace = ""
ignoreLastTransitionTime = cmpopts.IgnoreTypes(apis.Condition{}.LastTransitionTime.Inner.Time)
images = pipeline.Images{
EntrypointImage: "override-with-entrypoint:latest",
Expand Down
1 change: 0 additions & 1 deletion pkg/reconciler/taskrun/taskrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ const (
)

var (
namespace = "" // all namespaces
defaultActiveDeadlineSeconds = int64(config.DefaultTimeoutMinutes * 60 * 1.5)
images = pipeline.Images{
EntrypointImage: "override-with-entrypoint:latest",
Expand Down

0 comments on commit f8c2eea

Please sign in to comment.