Move loadGitDetails mutator to Initialize phase #1944
Conversation
denik
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
This will require API call and we want to keep Load phase fast.
denik
force-pushed
the
DECO-24125--move-git-load-to-initialize
branch
from
17f284e to
84d535a
Compare
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
| @@ -14,13 +14,15 @@ import ( | |||
|
|
|||
| func TestGitAutoLoad(t *testing.T) { | |||
| b := load(t, "./autoload_git") | |||
There was a problem hiding this comment.
Instead of load you can now directly use b := &bundle.Bundle{...} fixture.
There was a problem hiding this comment.
I see what you mean. However, I wonder, if it’s better to go through load process first anyway, since that’s always applied on actual Bundle before running this mutator and thus gives more realistic test.
I’d also wary about refactoring tests and functionality in the same PR, as purpose of the tests here is to check that this change is pure refactoring and does not affect behavior in unexpected way.
There was a problem hiding this comment.
In this case, the functionality is simple and localized enough that I think we'll be fine testing only the mutator since the functionality/context is localized to the load_git_details mutator.
But that's a personal preference, either way sounds reasonable enough.
| "fmt" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/databricks/cli/bundle" | ||
| "github.com/databricks/cli/bundle/config/mutator" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestGitAutoLoadWithEnvironment(t *testing.T) { |
There was a problem hiding this comment.
We no longer need tests for environment overrides if we move it out of the load phase. The environments are resolves by the time the code gets to the initialize phase.
There was a problem hiding this comment.
Can you explain why we needed this before and don't need this anymore? This PR does not change overall behavior, just reorders some operations.
In any case, if we think this test is not useful anymore I propose to clean it up in a follow up PR. In this PR it serves as proof that whatever it tests was not broken by this change.
There was a problem hiding this comment.
Can you explain why we needed this before and don't need this anymore?
Environments / Targets are not resolved in the DefaultMutators(). They are resolved right afterwards in the loading process. Since before we loaded the Git details in the loading phase it was relevant to test the environment.
The environments/targets block is already nil by the time the code gets to initialize phase.
|
Test Details: go/deco-tests/12108786336 |
This will require API call when run inside a workspace, which will require workspace client (we don't have one at the current point). We want to keep Load phase quick, since it's common across all commands.