remove unnecessary execution env vars #4806
Conversation
WalkthroughThe pull request modifies the environment variable handling in the Bacalhau system. Specifically, the changes remove the setting of Changes
Possibly related PRs
Poem
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/compute/envvars_test.go (2)
76-78: LGTM! Consider enhancing the test case.The test correctly verifies that task environment variables cannot override system environment variables. However, consider adding explicit assertions to verify that:
- Task-provided values for system environment variables are ignored
- All other task environment variables are passed through unchanged
Example enhancement:
t.Run(tt.name, func(t *testing.T) { got := GetExecutionEnvVars(tt.execution) assert.Equal(t, tt.want, got) + // Explicitly verify system env var wasn't overridden + assert.Equal(t, "batch", got["BACALHAU_JOB_TYPE"], "system env var should not be overridden") + // Verify custom env vars were passed through + assert.Equal(t, "my-value", got["MY_VAR"], "custom env var should be passed through") })
Line range hint
13-133: Consider adding a test case for concurrent executions.The test suite comprehensively covers various scenarios, but it might be valuable to add a test case that verifies environment variable isolation between concurrent executions of the same job. This would help ensure that environment variables are properly scoped and don't leak between executions.
Example test case to add:
{ name: "concurrent executions of same job", execution: &models.Execution{ ID: "exec-2", JobID: "job-1", PartitionIndex: 1, Job: &models.Job{ Type: "batch", Tasks: []*models.Task{{ Name: "task-1", Env: map[string]string{ "PARTITION_SPECIFIC_VAR": "value-2", }, }}, Count: 2, }, }, want: map[string]string{ "BACALHAU_EXECUTION_ID": "exec-2", "BACALHAU_JOB_ID": "job-1", "BACALHAU_JOB_TYPE": "batch", "BACALHAU_PARTITION_INDEX": "1", "BACALHAU_PARTITION_COUNT": "2", "PARTITION_SPECIFIC_VAR": "value-2", }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/compute/envvars_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testcontainers-suite / tests
Summary by CodeRabbit
NODE_ID,JOB_NAME, andJOB_NAMESPACEin job execution.