Rework artifact definition publishing #201
Conversation
chrisghill
requested review from
Copilot and
coryodaniel
and removed request for
coryodaniel
There was a problem hiding this comment.
Pull request overview
This PR refactors artifact definition publishing to improve the architecture and add YAML support. The key changes include moving dereferencing logic from the bundle package to the definition package, introducing a new Read function that handles both JSON and YAML formats, and simplifying the CLI interface to accept file paths instead of requiring stdin support.
- Moved dereferencing functionality from
pkg/bundle/dereference.gotopkg/definition/dereference.gofor better code organization - Created new
Readfunction that handles JSON/YAML parsing and dereferencing in one operation - Updated
Publishfunction to accept file paths directly instead ofio.Reader, simplifying the API - Removed
--fileflag from CLI in favor of positional arguments
Reviewed changes
Copilot reviewed 12 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/definition/read.go | New function to read and parse artifact definitions from JSON or YAML files with integrated dereferencing |
| pkg/definition/read_test.go | Tests for the new Read function covering both JSON and YAML formats |
| pkg/definition/dereference.go | Moved dereferencing logic from bundle package to definition package for better modularity |
| pkg/definition/dereference_test.go | Updated package references from bundle_test to definition_test |
| pkg/definition/publish.go | Refactored to use Read function and file paths instead of io.Reader |
| pkg/definition/publish_test.go | Updated tests to use file paths instead of buffers |
| pkg/bundle/dereference.go | Removed duplicate code and now imports from definition package |
| cmd/definition.go | Simplified CLI to use positional arguments instead of --file flag |
| cmd/schema.go | Updated imports to use definition package |
| docs/helpdocs/definition/publish.md | Updated documentation to reflect new positional argument syntax and YAML support |
| docs/generated/mass_definition_publish.md | Updated generated documentation with new usage examples |
| pkg/definition/testdata/* | Added test fixture files for JSON/YAML artifacts and dereferencing tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestRead(t *testing.T) { | ||
| want := map[string]any{ | ||
| "$schema": "http://json-schema.org/draft-07/schema", | ||
| "$md": map[string]any{ | ||
| "name": "foo", | ||
| }, | ||
| "type": "object", | ||
| "title": "Test Artifact", | ||
| "properties": map[string]any{ | ||
| "data": map[string]any{ | ||
| "type": "object", | ||
| }, | ||
| "specs": map[string]any{ | ||
| "type": "object", | ||
| }, | ||
| }, | ||
| } | ||
| type test struct { | ||
| name string | ||
| file string | ||
| } | ||
| tests := []test{ | ||
| { | ||
| name: "json", | ||
| file: filepath.Join("testdata", "simple-artifact.json"), | ||
| }, | ||
| { | ||
| name: "yaml", | ||
| file: filepath.Join("testdata", "simple-artifact.yaml"), | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| mdClient := client.Client{ | ||
| GQL: gqlmock.NewClientWithSingleJSONResponse(map[string]any{"data": map[string]any{}}), | ||
| } | ||
|
|
||
| got, err := definition.Read(context.Background(), &mdClient, tc.file) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| if !reflect.DeepEqual(got, want) { | ||
| t.Errorf("got %v, want %v", got, want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The TestRead function doesn't test error cases such as unsupported file extensions, malformed JSON/YAML, file not found errors, or dereferencing errors. Consider adding test cases for these scenarios to ensure proper error handling.
cmd/definition.go
Outdated
| } | ||
|
|
||
| definitionPublishCmd := &cobra.Command{ | ||
| Use: "publish", |
There was a problem hiding this comment.
The Use field for the publish command should include the expected argument in its usage string for consistency with the get command. Consider changing from "publish" to "publish [definition-file]" to match the pattern used in the get command and to provide better CLI help output.
| Use: "publish", | |
| Use: "publish [definition-file]", |
pkg/definition/publish_test.go
Outdated
| wantBody: `{"$md":{"access":"public","name":"foo"},"required":["data","specs"],"properties":{"data":{},"specs":{}}}`, | ||
| name: "simple", | ||
| path: "testdata/simple-artifact.json", | ||
| wantBody: `{"$schema":"http://json-schema.org/draft-07/schema","type":"object","title":"Test Artifact","properties":{"data":"type":"object"},"specs":"type":"object"}}}`, |
There was a problem hiding this comment.
The wantBody field contains malformed JSON with incorrect brace placement. The JSON has nested braces inside property values (e.g., "data":"type":"object") which is invalid. This field also appears to be unused in the test - consider either removing it or properly using it to validate the request body sent to the API.
No description provided.