Improve git config section validation #1940
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
denik
force-pushed
the
DECO-24125--no_override_commit
branch
from
53c07da to
a141e3f
Compare
denik
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
force-pushed
the
DECO-24125--no_override_commit
branch
from
a141e3f to
afc065f
Compare
denik
changed the title
denik
force-pushed
the
DECO-24125--no_override_commit
branch
from
afc065f to
39806c9
Compare
Previously we checked if ActualBranch (from .git) and Branch disagreed and issued an error unless --force was passed. We still do that, but also: - if --force is passed, we now log this as a warning (previously it went silent) - we do the same logic for commit (previously unchecked) - we do similar logic for originURL except it always logs warning and never prevent the deployment I removed separate mutator (validate_git_details.go) and put everything into load_git_details.go.
denik
force-pushed
the
DECO-24125--no_override_commit
branch
from
39806c9 to
23169cd
Compare
On CI info.CurrentBranch is "" but according to original logic it is still Inferred because it only checks if Config value is empty string.
This reverts commit e5a85e9.
| return []diag.Diagnostic{{ | ||
| Severity: severity, | ||
| Summary: fmt.Sprintf(tmpl, field, *configValue, fetchedValue, extra), | ||
| }} |
There was a problem hiding this comment.
This happens only if the configuration is explicitly set. Can you include the location information here? You can access it through b.Config.GetLocations() for the relevant paths in the configuration.
This means the user can Cmd-click on it to jump to the problematic location.
Separate: I see this matches the existing warning and that's fine, but this should really be broken out into a short single-line summary and longer multi-line "detail" field.
There was a problem hiding this comment.
I agree, this is a good change but I think it can be a follow up?
This PR focuses on the logic but keeps the message exactly the same (it just shows up in more places where relevant). Follow up PR can add location info and split summary/detail without changing the logic.
There was a problem hiding this comment.
I'd also prefer to have this addressed here. The PR adds additional logic for commit and url check anyway and thus the message is different, so having location info is useful.
Co-authored-by: Pieter Noordhuis <[email protected]>
|
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. |
|
Test Details: go/deco-tests/12240512958 |
Not, E2E, but there is https://github.com/databricks/cli/blob/main/bundle/tests/git_test.go#L45 that checks Branch validation behavior. |
There was a problem hiding this comment.
It is a potential breaking change because the validation used to happen after bundle variables were resolved but now it happens before. There's a chance (like in here #1255) that someone is using variable for Git option settings and thus validation will fail
| return diags | ||
| } | ||
|
|
||
| func checkMatch(field, fetchedValue string, configValue *string, allowedToMismatch bool) []diag.Diagnostic { |
There was a problem hiding this comment.
This should be renamed because it has side effects of setting configValue, not simply doing the check
There was a problem hiding this comment.
setIfEmptyAndCompare maybe? Which hints and maybe refactoring this into 2 separate functions but I'm fine with both
| return []diag.Diagnostic{{ | ||
| Severity: severity, | ||
| Summary: fmt.Sprintf(tmpl, field, *configValue, fetchedValue, extra), | ||
| }} |
There was a problem hiding this comment.
I'd also prefer to have this addressed here. The PR adds additional logic for commit and url check anyway and thus the message is different, so having location info is useful.
Previously we checked if ActualBranch (from .git) and Branch disagreed and issued an error unless --force was passed.
We still do that, but also:
I removed separate mutator (validate_git_details.go) and put everything into load_git_details.go.