-
Notifications
You must be signed in to change notification settings - Fork 130
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.
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
Apply dir overrides from agent config #894
Changes from all commits
cbfa1bb
acd0267
9047acd
10325f2
83e12f8
8c4c157
0ee6dcf
9d109e7
fe0af35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
bitrise_dirs: | ||
BITRISE_DATA_HOME_DIR: $INTEGRATION_TEST_BINARY_PATH/../agent | ||
BITRISE_SOURCE_DIR: $INTEGRATION_TEST_BINARY_PATH/../agent/workspace/$BITRISE_APP_SLUG | ||
BITRISE_DEPLOY_DIR: $INTEGRATION_TEST_BINARY_PATH/../agent/$BITRISE_APP_SLUG/$BITRISE_BUILD_SLUG/artifacts | ||
BITRISE_TEST_DEPLOY_DIR: $INTEGRATION_TEST_BINARY_PATH/../agent/$BITRISE_APP_SLUG/$BITRISE_BUILD_SLUG/test_results | ||
|
||
hooks: | ||
cleanup_on_workflow_start: | ||
- $BITRISE_DEPLOY_DIR | ||
|
||
cleanup_on_workflow_end: | ||
- $BITRISE_TEST_DEPLOY_DIR |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
package integration | ||
|
||
import ( | ||
"os" | ||
"testing" | ||
|
||
"github.com/bitrise-io/go-utils/command" | ||
"github.com/bitrise-io/go-utils/pathutil" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func Test_AgentConfigTest(t *testing.T) { | ||
cfg, err := os.ReadFile("agent-config.yml") | ||
require.NoError(t, err) | ||
|
||
absPath, err := pathutil.AbsPath("$HOME/.bitrise/agent-config.yml") | ||
require.NoError(t, err) | ||
|
||
err = os.WriteFile(absPath, cfg, 0644) | ||
require.NoError(t, err) | ||
defer func () { | ||
os.Remove(absPath) | ||
}() | ||
|
||
t.Setenv("BITRISE_APP_SLUG", "ef7a9665e8b6408b") | ||
t.Setenv("BITRISE_BUILD_SLUG", "80b66786-d011-430f-9c68-00e9416a7325") | ||
|
||
cmd := command.New(binPath(), "run", "test", "--config", "agent_config_test_bitrise.yml") | ||
out, err := cmd.RunAndReturnTrimmedCombinedOutput() | ||
require.NoError(t, err, out) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
format_version: 13 | ||
default_step_lib_source: https://github.com/bitrise-io/bitrise-steplib.git | ||
|
||
workflows: | ||
test: | ||
steps: | ||
- script: | ||
inputs: | ||
- content: | | ||
set -ex | ||
if [[ "$BITRISE_DATA_HOME_DIR" != $(dirname $INTEGRATION_TEST_BINARY_PATH)/agent ]]; then | ||
echo "BITRISE_DATA_HOME_DIR does not point to the expected directory" | ||
exit 1 | ||
fi | ||
|
||
ls $BITRISE_DATA_HOME_DIR | ||
|
||
if [[ "$BITRISE_SOURCE_DIR" != $(dirname $INTEGRATION_TEST_BINARY_PATH)/agent/workspace/$BITRISE_APP_SLUG ]]; then | ||
echo "BITRISE_SOURCE_DIR does not point to the expected directory" | ||
exit 1 | ||
fi | ||
|
||
ls $BITRISE_SOURCE_DIR | ||
|
||
if [[ "$BITRISE_DEPLOY_DIR" != $(dirname $INTEGRATION_TEST_BINARY_PATH)/agent/$BITRISE_APP_SLUG/$BITRISE_BUILD_SLUG/artifacts ]]; then | ||
echo "BITRISE_DEPLOY_DIR does not point to the expected directory" | ||
exit 1 | ||
fi | ||
|
||
ls $BITRISE_DEPLOY_DIR | ||
|
||
if [[ "$BITRISE_TEST_DEPLOY_DIR" != $(dirname $INTEGRATION_TEST_BINARY_PATH)/agent/$BITRISE_APP_SLUG/$BITRISE_BUILD_SLUG/test_results ]]; then | ||
echo "BITRISE_TEST_DEPLOY_DIR does not point to the expected directory" | ||
exit 1 | ||
fi | ||
|
||
ls $BITRISE_TEST_DEPLOY_DIR |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,10 +5,12 @@ import ( | |
"os" | ||
"path/filepath" | ||
|
||
"github.com/bitrise-io/bitrise/log" | ||
"github.com/bitrise-io/go-utils/pathutil" | ||
"gopkg.in/yaml.v2" | ||
) | ||
|
||
const agentConfigFileName = "agent-config.yml" | ||
const defaultSourceDir = "workspace" | ||
const defaultDeployDir = "$BITRISE_APP_SLUG/$BITRISE_BUILD_SLUG/artifacts" | ||
const defaultTestDeployDir = "$BITRISE_APP_SLUG/$BITRISE_BUILD_SLUG/test_results" | ||
|
@@ -53,6 +55,62 @@ type AgentHooks struct { | |
DoOnWorkflowEnd string `yaml:"do_on_workflow_end"` | ||
} | ||
|
||
func RegisterAgentOverrides() error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you, I fully agree, please take a look now |
||
if !hasAgentConfigFile() { | ||
return nil | ||
} | ||
|
||
file := filepath.Join(GetBitriseHomeDirPath(), agentConfigFileName) | ||
|
||
log.Print("") | ||
log.Info("Running in agent mode") | ||
log.Printf("Config file: %s", file) | ||
|
||
config, err := readAgentConfig(file) | ||
if err != nil { | ||
return fmt.Errorf("agent config file: %w", err) | ||
} | ||
|
||
params := []struct { | ||
dir string | ||
envKey string | ||
}{ | ||
{ | ||
dir: config.BitriseDirs.BitriseDataHomeDir, | ||
envKey: BitriseDataHomeDirEnvKey, | ||
}, | ||
{ | ||
dir: config.BitriseDirs.SourceDir, | ||
envKey: BitriseSourceDirEnvKey, | ||
}, | ||
{ | ||
dir: config.BitriseDirs.DeployDir, | ||
envKey: BitriseDeployDirEnvKey, | ||
}, | ||
{ | ||
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.envKey, err) | ||
} | ||
err = os.Setenv(param.envKey, param.dir) | ||
if err != nil { | ||
return fmt.Errorf("set %s: %w", param.envKey, err) | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func hasAgentConfigFile() bool { | ||
exists, _ := pathutil.IsPathExists(filepath.Join(GetBitriseHomeDirPath(), agentConfigFileName)) | ||
return exists | ||
} | ||
|
||
func readAgentConfig(configFile string) (AgentConfig, error) { | ||
fileContent, err := os.ReadFile(configFile) | ||
if err != nil { | ||
|
@@ -65,76 +123,80 @@ func readAgentConfig(configFile string) (AgentConfig, error) { | |
return AgentConfig{}, err | ||
} | ||
|
||
expandedBitriseDataHomeDir, err := expandPath(config.BitriseDirs.BitriseDataHomeDir) | ||
dataHomeDir, err := normalizePath(config.BitriseDirs.BitriseDataHomeDir) | ||
if err != nil { | ||
return AgentConfig{}, fmt.Errorf("expand BITRISE_DATA_HOME_DIR value: %s", err) | ||
} | ||
config.BitriseDirs.BitriseDataHomeDir = expandedBitriseDataHomeDir | ||
config.BitriseDirs.BitriseDataHomeDir = dataHomeDir | ||
|
||
// BITRISE_SOURCE_DIR | ||
if config.BitriseDirs.SourceDir == "" { | ||
config.BitriseDirs.SourceDir = filepath.Join(config.BitriseDirs.BitriseDataHomeDir, defaultSourceDir) | ||
} | ||
expandedSourceDir, err := expandPath(config.BitriseDirs.SourceDir) | ||
sourceDir, err := normalizePath(config.BitriseDirs.SourceDir) | ||
if err != nil { | ||
return AgentConfig{}, fmt.Errorf("expand BITRISE_SOURCE_DIR value: %s", err) | ||
} | ||
config.BitriseDirs.SourceDir = expandedSourceDir | ||
config.BitriseDirs.SourceDir = sourceDir | ||
|
||
// BITRISE_DEPLOY_DIR | ||
if config.BitriseDirs.DeployDir == "" { | ||
config.BitriseDirs.DeployDir = filepath.Join(config.BitriseDirs.BitriseDataHomeDir, defaultDeployDir) | ||
} | ||
expandedDeployDir, err := expandPath(config.BitriseDirs.DeployDir) | ||
deployDir, err := normalizePath(config.BitriseDirs.DeployDir) | ||
if err != nil { | ||
return AgentConfig{}, fmt.Errorf("expand BITRISE_DEPLOY_DIR value: %s", err) | ||
} | ||
config.BitriseDirs.DeployDir = expandedDeployDir | ||
config.BitriseDirs.DeployDir = deployDir | ||
|
||
// BITRISE_TEST_DEPLOY_DIR | ||
if config.BitriseDirs.TestDeployDir == "" { | ||
config.BitriseDirs.TestDeployDir = filepath.Join(config.BitriseDirs.BitriseDataHomeDir, defaultTestDeployDir) | ||
} | ||
expandedTestDeployDir, err := expandPath(config.BitriseDirs.TestDeployDir) | ||
testDeployDir, err := normalizePath(config.BitriseDirs.TestDeployDir) | ||
if err != nil { | ||
return AgentConfig{}, fmt.Errorf("expand BITRISE_TEST_DEPLOY_DIR value: %s", err) | ||
} | ||
config.BitriseDirs.TestDeployDir = expandedTestDeployDir | ||
config.BitriseDirs.TestDeployDir = testDeployDir | ||
|
||
// Hooks | ||
if config.Hooks.DoOnWorkflowStart != "" { | ||
expandedDoOnWorkflowStart, err := expandPath(config.Hooks.DoOnWorkflowStart) | ||
doOnWorkflowStart, err := normalizePath(config.Hooks.DoOnWorkflowStart) | ||
if err != nil { | ||
return AgentConfig{}, fmt.Errorf("expand do_on_workflow_start value: %s", err) | ||
} | ||
doOnWorkflowStartExists, err := pathutil.IsPathExists(expandedDoOnWorkflowStart) | ||
doOnWorkflowStartExists, err := pathutil.IsPathExists(doOnWorkflowStart) | ||
if err != nil { | ||
return AgentConfig{}, err | ||
} | ||
if !doOnWorkflowStartExists { | ||
return AgentConfig{}, fmt.Errorf("do_on_workflow_start path does not exist: %s", expandedDoOnWorkflowStart) | ||
return AgentConfig{}, fmt.Errorf("do_on_workflow_start path does not exist: %s", doOnWorkflowStart) | ||
} | ||
config.Hooks.DoOnWorkflowStart = expandedDoOnWorkflowStart | ||
config.Hooks.DoOnWorkflowStart = doOnWorkflowStart | ||
} | ||
|
||
if config.Hooks.DoOnWorkflowEnd != "" { | ||
expandedDoOnWorkflowEnd, err := expandPath(config.Hooks.DoOnWorkflowEnd) | ||
doOnWorkflowEnd, err := normalizePath(config.Hooks.DoOnWorkflowEnd) | ||
if err != nil { | ||
return AgentConfig{}, fmt.Errorf("expand do_on_workflow_end value: %s", err) | ||
} | ||
doOnWorkflowEndExists, err := pathutil.IsPathExists(expandedDoOnWorkflowEnd) | ||
doOnWorkflowEndExists, err := pathutil.IsPathExists(doOnWorkflowEnd) | ||
if err != nil { | ||
return AgentConfig{}, err | ||
} | ||
if !doOnWorkflowEndExists { | ||
return AgentConfig{}, fmt.Errorf("do_on_workflow_end path does not exist: %s", expandedDoOnWorkflowEnd) | ||
return AgentConfig{}, fmt.Errorf("do_on_workflow_end path does not exist: %s", doOnWorkflowEnd) | ||
} | ||
config.Hooks.DoOnWorkflowEnd = expandedDoOnWorkflowEnd | ||
config.Hooks.DoOnWorkflowEnd = doOnWorkflowEnd | ||
} | ||
|
||
return config, nil | ||
} | ||
|
||
func expandPath(path string) (string, error) { | ||
return pathutil.ExpandTilde(os.ExpandEnv(path)) | ||
func normalizePath(path string) (string, error) { | ||
expanded, err := pathutil.ExpandTilde(os.ExpandEnv(path)) | ||
if err != nil { | ||
return "", err | ||
} | ||
return pathutil.AbsPath(expanded) | ||
} |
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.
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:
bitrise/cli/run.go
Line 88 in cbfa1bb
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree with you that this is
bitrise run
specific. The code I linked is in therun.go
file which is hosting thebitrise run
subcommand:bitrise/cli/run.go
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 therun
subcommand execution could make sense.