feat: add schema cmds #37
Conversation
Co-authored-by: Eric Wyles <23637493+ericwyles@users.noreply.github.com>
| github.com/spf13/cobra v1.8.1 | ||
| github.com/stretchr/testify v1.9.0 | ||
| github.com/xanzy/go-gitlab v0.112.0 | ||
| github.com/yudai/gojsondiff v1.0.0 |
There was a problem hiding this comment.
do we need this? The package is 7 yrs old
There was a problem hiding this comment.
We shouldnt need it i'll look at finding a different way or library.
|
|
||
| } |
| schemaCmd.AddCommand(generateSchemas()) | ||
| schemaCmd.AddCommand(checkSchemas()) | ||
| rootCmd.AddCommand(schemaCmd) |
There was a problem hiding this comment.
we should be consistent with the patterns we use for this (see how the other pk commands are)
There was a problem hiding this comment.
Sure that's an easy enough change.
| if _, err := os.Stat(outputFile); errors.Is(err, os.ErrNotExist) { | ||
| message.Warnf("Existing schema not found at %s\n", outputFile) | ||
| differencesFound = true | ||
| continue | ||
| } | ||
| existingSchema, err := os.ReadFile(outputFile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Compare schemas | ||
| generatedSchema, err := json.MarshalIndent(baseSchema, "", " ") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !strings.EqualFold(string(existingSchema), string(generatedSchema)) { | ||
| message.Warnf("Schemas do not match for %s.\nDifferences:\n", valuesFile) | ||
| fmt.Println(diffSchemas(string(existingSchema), string(generatedSchema))) | ||
| differencesFound = true | ||
| } |
There was a problem hiding this comment.
We may want to handle non-existence and differences differently - I kind of expect a few cases where we may want to fail harder or just warn depending on what is happening:
- No schema - this should likely be a fail hard (we can grace period it for a bit in uds-common but folks should have schemas IMO and if we make it easy enough to add them it should be an easy add)
- Schema deletions - if keys are missing from the other schema this should likely be a fail as well since we should be able to be confident that the keys we see are supposed to be there
- Schema additions - if keys have been added this may make more sense as a warning bc folks may have added things to their schema (though this may be rare in practice)
Regardless of that though we likely want to fail harder for now bc a downstream script will have difficulty seeing whether something failed or not (it would need to parse the CLI) - if we want to do grace periods we can handle that in the actions / scripts that call this and add the right annotations to bring that to the user's attention.
| if customVal, exists := custom[k]; exists { | ||
| props[k] = customVal |
There was a problem hiding this comment.
does this actually work when things are nested? (i.e. replacing something like sso.mappers) - k would always be a single-deep key name without the hierarchy and if the custom map just had mappers in it it might match other things and replace them erroneously.
There was a problem hiding this comment.
I'll double check the rest but it was working with this
kubernetesSandbox.additionalNetworkAllow and just additionalNetworkAllow
Or are you saying it might be too aggressive in replacing?
| // After replacement, key1 should be replaced with custom schema | ||
| if ((base["properties"].(map[string]interface{})["key1"]).(map[string]interface{})["type"]) != "number" { | ||
| t.Errorf("expected key1 type to be number after replacement") | ||
| } | ||
|
|
||
| // key2 remains unchanged since it's not in custom | ||
| nestedProps := (base["properties"].(map[string]interface{})["nested"]).(map[string]interface{})["properties"].(map[string]interface{}) | ||
| if nestedProps["key2"].(map[string]interface{})["type"] != "string" { | ||
| t.Errorf("expected key2 to remain string") | ||
| } |
There was a problem hiding this comment.
this is only testing a single layer not the recursive flow
|
|
||
|
|
There was a problem hiding this comment.
where do these spaces come from?
| err := e2e.CopyDir(srcDir, filepath.Join(sandboxDir, test)) | ||
| require.NoError(t, err, "failed to copy test files into sandbox") | ||
| stdout, stderr, err := e2e.UDSPK("schema", "validate", "-b", dir) | ||
| require.NoError(t, err, stdout, stderr) |
There was a problem hiding this comment.
I think this should error so that we can handle it in the calling script to add GH annotations or other things during the grace period
| # parameters later on so any nested objects/arrays should be encoded as a | ||
| # string query parameter instead of their full type. | ||
| application: | ||
| default_snippet_visibility: private |
There was a problem hiding this comment.
we will need to figure out how to handle additionalProperties on these nested objects - this is not all of the options for GitLab in this case and it shouldn't fail if a user adds an additional property here
Co-authored-by: Wayne Starr <Racer159@users.noreply.github.com>
No description provided.