-
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 7 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,72 @@ 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) | ||
} | ||
log.Printf("env: %s", os.Getenv(BitriseDataHomeDirEnvKey)) | ||
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. Is this a leftover print statement? Just asking because when it is executed it will print out something like
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it was just a leftover debug statement |
||
|
||
// BITRISE_DATA_HOME_DIR | ||
err = pathutil.EnsureDirExist(config.BitriseDirs.BitriseDataHomeDir) | ||
if err != nil { | ||
return fmt.Errorf("can't create BITRISE_DATA_HOME_DIR: %w", err) | ||
} | ||
err = os.Setenv(BitriseDataHomeDirEnvKey, config.BitriseDirs.BitriseDataHomeDir) | ||
if err != nil { | ||
return fmt.Errorf("set BITRISE_DATA_HOME_DIR: %w", err) | ||
} | ||
|
||
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. super nit: there are 2 newlines here while everywhere else in the same file there is only 1. 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. Fixed |
||
|
||
// BITRISE_SOURCE_DIR | ||
err = pathutil.EnsureDirExist(config.BitriseDirs.SourceDir) | ||
if err != nil { | ||
return fmt.Errorf("can't create BITRISE_SOURCE_DIR: %w", err) | ||
} | ||
err = os.Setenv(BitriseSourceDirEnvKey, config.BitriseDirs.SourceDir) | ||
if err != nil { | ||
return fmt.Errorf("set BITRISE_SOURCE_DIR: %w", err) | ||
} | ||
|
||
// BITRISE_DEPLOY_DIR | ||
err = pathutil.EnsureDirExist(config.BitriseDirs.DeployDir) | ||
if err != nil { | ||
return fmt.Errorf("can't create BITRISE_DEPLOY_DIR: %w", err) | ||
} | ||
os.Setenv(BitriseDeployDirEnvKey, config.BitriseDirs.DeployDir) | ||
if err != nil { | ||
return fmt.Errorf("set BITRISE_DEPLOY_DIR: %w", err) | ||
} | ||
|
||
// BITRISE_TEST_DEPLOY_DIR | ||
err = pathutil.EnsureDirExist(config.BitriseDirs.TestDeployDir) | ||
if err != nil { | ||
return fmt.Errorf("can't create BITRISE_TEST_DEPLOY_DIR: %w", err) | ||
} | ||
os.Setenv(BitriseTestDeployDirEnvKey, config.BitriseDirs.TestDeployDir) | ||
if err != nil { | ||
return fmt.Errorf("set BITRISE_TEST_DEPLOY_DIR: %w", 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 +133,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.