Implement half of the CLI #8
Conversation
AislingHPE
commented
Aug 22, 2024
AislingHPE
commented
- Adds and implements --require-all and --disallow-additional-properties
- Adds checks to make sure that the input folder specified is actually a folder
- Generally tidies up some comments and minor parts of the code, such as logging.
- Refactors cmd.go into multiple functions
AislingHPE
force-pushed
the
add-cli-1
branch
3 times, most recently
from
bdfe7c6 to
4c2dc11
Compare
- Adds and implements --require-all and --disallow-additional-properties - Adds checks to make sure that the input folder specified is actually a folder - Generally tidies up some comments and minor parts of the code, such as logging. - Refactors cmd.go into multiple functions
cmd/cmd.go
Outdated
| Use: "terraschema", | ||
| Example: "terraschema -i /path/to/module -o /path/to/schema.json", | ||
| Short: "Generate JSON schema from HCL Variable Blocks in a Terraform/OpenTofu module", | ||
| Long: `TODO: Long description`, |
There was a problem hiding this comment.
Is it worth filling in the long description as part of this pr?
There was a problem hiding this comment.
I was waiting until I'd acutally written the documentation but I can add it here now that that exists.
cmd/cmd.go
Outdated
| // TODO: implement | ||
| rootCmd.Flags().StringVarP(&output, "output", "o", "schema.json", "output file path for schema") | ||
| rootCmd.Flags().StringVarP(&output, "output", "o", "schema.json", | ||
| "output file path for schema", |
There was a problem hiding this comment.
suggestion: "for json schema"?
cmd/cmd.go
Outdated
|
|
||
| folder, err := os.Stat(path) | ||
| if err != nil { | ||
| errReturned = fmt.Errorf("could not open %q: %w", path, err) |
There was a problem hiding this comment.
This is probably a directory? Maybe 'access' rather than 'open'? (Not a big deal either way, obviously)
| return | ||
| } | ||
|
|
||
| if !folder.IsDir() { |
There was a problem hiding this comment.
fwiw this works if input is a sym link to a directory (due to using Stat).
| return | ||
| } | ||
|
|
||
| fmt.Println(string(jsonOutput)) |
There was a problem hiding this comment.
This is going to stdout (good default behaviour), so "-o" flag not implemented? (Or did I miss something?)
There was a problem hiding this comment.
Implemented in the PR after this
| @@ -283,7 +287,7 @@ func TestSampleInput(t *testing.T) { | |||
|
|
|||
| input, err := os.ReadFile(tc.filePath) | |||
| require.NoError(t, err) | |||
| var m interface{} | |||
| var m any | |||
There was a problem hiding this comment.
I'm curious - this change is just a style thing?
pkg/jsonschema/type.go
Outdated
| node := map[string]any{ | ||
| "type": "object", | ||
| } | ||
| if strict { | ||
| if !options.AllowAdditionalProperties { |
There was a problem hiding this comment.
Do we need the ! in the if statement? ie this may scan a little easier without the negation.
There was a problem hiding this comment.
kicking myself for this one now. In my defence there were 2 PRs for this line that i put up in different orders.
| // For the purpose of this application, all that matters is the model.VariableBlock contained in this, which | ||
| // contains a direct unmarshal of the block itself using the hcl package. The rest of the information is for | ||
| // debugging purposes, and to simplify the process of deciding if a variable is 'required' later. Note: in 'strict' | ||
| // mode, all variables are required, regardless of whether they have a default value or not. |
| @@ -28,10 +28,10 @@ func TestGetVarMap_Required(t *testing.T) { | |||
|
|
|||
| for k, v := range varMap { | |||
| if v.Required && v.Variable.Default != nil { | |||
| t.Errorf("Variable %s is required but has a default", k) | |||
| t.Errorf("Variable %q is required but has a default", k) | |||
There was a problem hiding this comment.
I generally use capitals in t.Errorf statements. There isn't really any reason for that but I also think it's okay to leave it that way. Mainly because this is 'just the tests'
| } | ||
| if !v.Required && v.Variable.Default == nil { | ||
| t.Errorf("Variable %s is not required but has no default", k) | ||
| t.Errorf("Variable %q is not required but has no default", k) |
