-
Notifications
You must be signed in to change notification settings - Fork 128
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
feat: support online validation with Konnect #1335
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1335 +/- ##
==========================================
- Coverage 29.14% 28.99% -0.16%
==========================================
Files 59 59
Lines 5215 5243 +28
==========================================
Hits 1520 1520
- Misses 3587 3615 +28
Partials 108 108 ☔ View full report in Codecov by Sentry. |
@@ -181,6 +181,9 @@ func (v *Validator) Validate(formatVersion semver.Version) []error { | |||
if err := v.entities(v.state.FilterChains, "filter_chains"); err != nil { | |||
allErr = append(allErr, err...) | |||
} | |||
if err := v.entities(v.state.Vaults, "vaults"); err != nil { |
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.
Sorry, I lack some context here. How is this addition related to this PR? @GGabriele
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.
ping: @GGabriele. Do you mind chiming in here? 🙇
Also, this change adds a conditional that ensures that rbac flag used with konnect mode, gives an legible error to the user.
Other e2e tests for deck validate commands are added here: #1415 Kept this PR scoped to Konnect stuff. |
@pmalek Could you please help us with a review here? We have extended |
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.
I've left some minor comments but to me this looks pretty solid 👍
cmdErr := deckCmd.ExecuteContext(context.Background()) | ||
|
||
return cmdErr |
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.
cmdErr := deckCmd.ExecuteContext(context.Background()) | |
return cmdErr | |
return deckCmd.ExecuteContext(context.Background()) |
validateOpts := []string{ | ||
tc.stateFile, | ||
} | ||
validateOpts = append(validateOpts, tc.additionalArgs...) |
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.
validateOpts := []string{ | |
tc.stateFile, | |
} | |
validateOpts = append(validateOpts, tc.additionalArgs...) | |
validateOpts = append([]string{tc.stateFile}, tc.additionalArgs...) |
} | ||
} else { | ||
assert.NoError(t, err) | ||
} |
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.
} | |
} else { | |
assert.NoError(t, err) | |
} | |
} | |
return | |
} | |
assert.NoError(t, err) |
@@ -374,3 +374,23 @@ func render(opts ...string) (string, error) { | |||
|
|||
return stripansi.Strip(string(out)), cmdErr | |||
} | |||
|
|||
func validate(online bool, opts ...string) error { |
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.
Nit: might as well make the first parameter its own type so that we can call it like:
validate(Online, args...)
Instead of passing bool explicitly.
No description provided.