Apply dir overrides from agent config #894
Conversation
log/log.go
Outdated
| m.Print("") | ||
| m.Printf("Invocation started at %s", colorstring.Cyan(m.opts.TimeProvider().Format(consoleTimeLayout))) | ||
| m.Printf("Bitrise CLI version: %s", colorstring.Cyan(plan.Version)) | ||
| m.Print("") |
There was a problem hiding this comment.
nit: you can save some time by omitting the "" parameter to print a newline. It will work with an empty argument because the logger will append a \n char to whatever you supply here. And supplying nothing will end up with juts the single \n char.
configs/agent_config.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("agent config file: %w", err) | ||
| } | ||
| log.Printf("env: %s", os.Getenv(BitriseDataHomeDirEnvKey)) |
There was a problem hiding this comment.
Is this a leftover print statement? Just asking because when it is executed it will print out something like
env: /path/to/some/folder
and if the folder does not have a clear name then (without knowing the CLI codebase) it will be hard to understand for the users what env is being printed out.
So I think we should either add the env name into the print statement or remove it.
There was a problem hiding this comment.
Yes, it was just a leftover debug statement
configs/agent_config.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("set BITRISE_DATA_HOME_DIR: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
super nit: there are 2 newlines here while everywhere else in the same file there is only 1.
log/log.go
Outdated
| if plan.DebugMode { | ||
| m.Printf("Debug mode: %v", colorstring.Cyan("%v", plan.DebugMode)) | ||
| } |
There was a problem hiding this comment.
nit: I think it is perfectly fine to always print out if we are in debug mode or not.
There was a problem hiding this comment.
Yes, good point, if for nothing else, this introduces people to the debug flag 😆
| @@ -53,6 +55,72 @@ type AgentHooks struct { | |||
| DoOnWorkflowEnd string `yaml:"do_on_workflow_end"` | |||
| } | |||
|
|
|||
| func RegisterAgentOverrides() error { | |||
There was a problem hiding this comment.
I had to closely look at and compare all of 4 sections of the dir handling sections inside this function to discover that they are exactly the same. This made me wonder if it would be good to group them and create a single logic with multiple input parameters for them to indicate that they are the same.
Something like
params := []struct {
name string
dir string
envKey string
}{
{
name: "BITRISE_DATA_HOME_DIR",
dir: config.BitriseDirs.BitriseDataHomeDir,
envKey: BitriseDataHomeDirEnvKey,
},
{
name: "BITRISE_SOURCE_DIR",
dir: config.BitriseDirs.SourceDir,
envKey: BitriseSourceDirEnvKey,
},
{
name: "BITRISE_DEPLOY_DIR",
dir: config.BitriseDirs.DeployDir,
envKey: BitriseDeployDirEnvKey,
},
{
name: "BITRISE_TEST_DEPLOY_DIR",
dir: config.BitriseDirs.TestDeployDir,
envKey: BitriseTestDeployDirEnvKey,
},
}
for _, param := range params {
err = pathutil.EnsureDirExist(param.dir)
if err != nil {
return fmt.Errorf("can't create %s: %w", param.name, err)
}
err = os.Setenv(param.envKey, param.dir)
if err != nil {
return fmt.Errorf("set %s: %w", param.name, err)
}
}
There was a problem hiding this comment.
Thank you, I fully agree, please take a look now
| if err := configs.RegisterAgentOverrides(); err != nil { | ||
| return models.BuildRunResultsModel{}, fmt.Errorf("apply agent config: %s", err) | ||
| } |
There was a problem hiding this comment.
Now this checks happens in the workflow execution process but to me this seems like not directly linked to it. If we can then I would put this somewhere earlier in the chain. Like as high level as we can put it. Maybe before this line:
Line 88 in cbfa1bb
There was a problem hiding this comment.
Interesting, my mental model is that all of these agent-specific modifications are only relevant to the bitrise run subcommand. Do you think it should apply to other subcommands too?
There was a problem hiding this comment.
Yes, I agree with you that this is bitrise run specific. The code I linked is in the run.go file which is hosting the bitrise run subcommand:
Lines 50 to 71 in cbfa1bb
What I was referring to is that where this logic is now is a place where the actual execution flow is calculated inside the WorkflowRunner struct. This agent dir override seemed like something this runner should not be even aware and that is why I thought moving it out to the very beginning of the run subcommand execution could make sense.
Checklist
README.mdis updated with the changes (if needed)Version
Requires a MINOR* version update
Context
Follow-up to #892, this PR is about applying the defined Bitrise dir overrides when running a workflow
Changes
Investigation details
Decisions
Applying the other configs from agent-config.yml is out of scope for this PR