-
Notifications
You must be signed in to change notification settings - Fork 327
cli/pipeline_run: Include variables in job template for pipeline run #4137
Conversation
Prior to this commit, when a pipeline was run, the CLI variables from the job client were not included in the submitted job template that the CLI would send to the server to run pipeline jobs. This means any CLI variables would not be properly resolved when the job went to execute. This commit fixes that by including those variables in the job template. Fixes #4134
@@ -99,6 +99,7 @@ func (c *PipelineRunCommand) Run(args []string) int { | |||
Application: app.Ref(), | |||
Workspace: c.project.WorkspaceRef(), | |||
TargetRunner: &pb.Ref_Runner{Target: &pb.Ref_Runner_Any{}}, | |||
Variables: c.variables, |
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.
I haven't dug deeper so it might be there, but I don't see the code where these variables are then handled?
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.
When you initialize a CLI client, we evaluate variables and load them onto the base client to be made available to any CLI clients:
Lines 289 to 299 in b7c98b1
// Collect variable values from -var and -varfile flags, | |
// and env vars set with WP_VAR_* and set them on the job | |
vars, diags := variables.LoadVariableValues(c.flagVars, c.flagVarFile) | |
if diags.HasErrors() { | |
// we only return errors for file parsing, so we are specific | |
// in the error log here | |
c.logError(c.Log, "failed to load wpvars file", errors.New(diags.Error())) | |
return diags | |
} | |
c.variables = vars |
Configuring a basic CLI client gets these variables attached to it (i.e. c.variables
)
waypoint/internal/cli/base_init.go
Lines 95 to 104 in b7c98b1
// Start building our client options | |
opts := []clientpkg.Option{ | |
clientpkg.WithLogger(c.Log), | |
clientpkg.WithClientConnect(connectOpts...), | |
clientpkg.WithProjectRef(c.refProject), | |
clientpkg.WithWorkspaceRef(c.refWorkspace), | |
clientpkg.WithVariables(c.variables), | |
clientpkg.WithLabels(c.flagLabels), | |
clientpkg.WithSourceOverrides(c.flagRemoteSource), | |
} |
CLI commands like waypoint up
, waypoint deploy
, etc use DoApp to run app operations directly, and when that happens, the job client gets those variables attached to the job template:
waypoint/internal/client/job.go
Lines 22 to 42 in b7c98b1
// job returns the basic job skeleton pre-populated with the correct | |
// defaults based on how the client is configured. For example, for local | |
// operations, this will already have the targeting for the local runner. | |
func (c *Project) job() *pb.Job { | |
job := &pb.Job{ | |
TargetRunner: c.runner, | |
Labels: c.labels, | |
Variables: c.variables, | |
Workspace: c.workspace, | |
Application: &pb.Ref_Application{ | |
Project: c.project.Project, | |
}, | |
DataSourceOverrides: c.dataSourceOverrides, | |
Operation: &pb.Job_Noop_{ | |
Noop: &pb.Job_Noop{}, | |
}, | |
} | |
return job | |
} |
The waypoint pipeline run
CLI is a bit different because it sends the job template off to the server to be used in the job generater when resolving pipeline steps to queued job requests. So the fix here is including those init CLI variables on the job template!
waypoint/internal/cli/pipeline_run.go
Lines 97 to 103 in 1698c1d
// build the initial job template for running the pipeline | |
runJobTemplate := &pb.Job{ | |
Application: app.Ref(), | |
Workspace: c.project.WorkspaceRef(), | |
TargetRunner: &pb.Ref_Runner{Target: &pb.Ref_Runner_Any{}}, | |
Variables: c.variables, | |
} |
Prior to this commit, when a pipeline was run, the CLI variables from the job client were not included in the submitted job template that the CLI would send to the server to run pipeline jobs. This means any CLI variables would not be properly resolved when the job went to execute. This commit fixes that by including those variables in the job template.
Fixes #4134