FSOC - Table for config and patch flag implemented #146
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* melt model support for isolation. * fixing make pre-commit issue
- Enhance logging of profiler report processing
Attempt to fix FSOC-100, oauth login may show "request failed" in browser (even though the login was successful) instead of "login completed successfully". Co-authored-by: GDW1 <guwilks@cisco.com>
on optimizer Orion config object
* Unhide --subscribe flag * Fix issue when --subscribe is used with --wait * Do not allow --subscribe with --solution-bundle * Remove double print of messages upon subscribe * Make subscription retries more resiliant (up to 4 retries with backoff) Co-authored-by: Pavel Georgiev <p3l@appdynamics.com>
…heading level if the --h1 flag used (cisco-open#143) * Filter and Print options for queries. example shown with fsoc solution list * fix so that generating gendocs starts on the first level of heading * hid gendocs header functionality behind header --------- Co-authored-by: GDW1 <guwilks@cisco.com>
* FSOC-37 Added argument support to * fixed deprecation messages, fixed documentation examples --------- Co-authored-by: GDW1 <guwilks@cisco.com>
pnickolov
requested changes
Jul 12, 2023
cmd/config/set.go
Outdated
| if val != "" && !slices.Contains(GetAuthMethodsStringList(), val) { | ||
| log.Fatalf(`Invalid --auth method %q; must be one of {"%v"}`, val, strings.Join(GetAuthMethodsStringList(), `", "`)) | ||
| } | ||
| //If this auth is different from the last auth then clear all fields |
There was a problem hiding this comment.
Suggested change
| //If this auth is different from the last auth then clear all fields | |
| //If this auth is different from the last auth then clear all subordinate auth settings |
cmd/config/set.go
Outdated
| @@ -195,12 +209,21 @@ func configSetContext(cmd *cobra.Command, args []string) { | |||
| if err != nil { | |||
| log.Fatal(err.Error()) | |||
| } | |||
| if !flags.Changed("patch") { | |||
There was a problem hiding this comment.
use the bool value of the flag, not whether it was set on the command line
cmd/config/set.go
Outdated
| @@ -187,6 +200,7 @@ func configSetContext(cmd *cobra.Command, args []string) { | |||
| log.Fatal(err.Error()) | |||
| } | |||
| log.Warnf("The --server option is now deprecated. In the future, please use --url instead. We will set the url to %q for you now", cleanedUrl) | |||
| URLClearConditions(ctxPtr) | |||
There was a problem hiding this comment.
shouldn't this be done only if not patching?
cmd/config/set.go
Outdated
| @@ -273,3 +326,13 @@ func expandHomePath(file string) string { | |||
| } | |||
| return file | |||
| } | |||
|
|
|||
| func clearContext(ctxPtr *Context) { | |||
There was a problem hiding this comment.
consider renaming to clearAuthFields()
In the future, the context may contain non-auth-related values (e.g., devenv or uql.apiver). What we want to wipe are only the authentication fields that are likely no longer applicable.
cmd/config/set.go
Outdated
| } | ||
| //If this auth is different from the last auth then clear all fields | ||
| if val != ctxPtr.AuthMethod { // deal with conditions here | ||
| clearContext(ctxPtr) |
There was a problem hiding this comment.
Suggested change
| clearContext(ctxPtr) | |
| clearContext(ctxPtr) // must be done before any of the other fields are set in the context |
cmd/config/set.go
Outdated
| ctxPtr.URL = cleanedUrl | ||
| } | ||
| if flags.Changed("tenant") { | ||
| if ctxPtr.AuthMethod != "jwt" && !flags.Changed("patch") { |
There was a problem hiding this comment.
actually, the tenant is allowed for the JWT auth mode. Also the check above is for non-JWT mode... Can you please check/re-consider here?
cmd/config/set.go
Outdated
| ctxPtr.Tenant, _ = flags.GetString("tenant") | ||
| } | ||
| if flags.Changed("token") { | ||
| if ctxPtr.AuthMethod != "jwt" && !flags.Changed("patch") { | ||
| log.Fatalf("The tenant cannot be set for jwt. To override this, use the --patch flag") |
There was a problem hiding this comment.
see note above. Token can be explicitly set ONLY for the JWT mode, so the message seems incorrect
cmd/config/set.go
Outdated
|
|
||
| if ctxPtr.AuthMethod == AuthMethodLocal { | ||
| if flags.Changed(AppdPid) { | ||
| if ctxPtr.AuthMethod != "service-principal" && ctxPtr.AuthMethod != "agent-principal" && !flags.Changed("patch") { |
There was a problem hiding this comment.
unnecessary/confusing check. We already know we're in AuthMethodLocal auth mode (see line 250 above)
cmd/config/set.go
Outdated
| } | ||
| } | ||
|
|
||
| func TennantClearConditions(ctxPtr *Context) { |
There was a problem hiding this comment.
Suggested change
| func TennantClearConditions(ctxPtr *Context) { | |
| func TenantClearConditions(ctxPtr *Context) { |
There was a problem hiding this comment.
Also, this seems to be the same as the URLClearConditions.
GDW1
force-pushed
the
FSOC-Config-Automation
branch
from
4d86333
to
df69ef2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When executing a config set, it will check if the current authentication type allows writing to this field (can be overriden), additionally when changing the authentication type, it will clear all fields.
Type of Change
Checklist