-
Notifications
You must be signed in to change notification settings - Fork 54
ref(create): switch default file format to JSON #536
Conversation
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.
Tiny docs niggle
cmd/duffle/create.go
Outdated
|
||
For example, 'duffle create foo' will create a directory structure that looks like this: | ||
|
||
foo/ | ||
| | ||
|- duffle.yaml # Contains metadata for bundle | ||
|- duffle.json # Contains metadata for bundle |
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.
Would you describe this as 'metadata'? I think I see where you're coming from, but it feels like more the bundle definition or bundle build source or something like that to me.
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.
Good suggestion. What do you think about "The bundle build definition file"? Still seems kinda vague... Any suggestions?
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.
That's fine I think. I don't think it's too vague and we don't have space to go into more detail.
pkg/duffle/manifest/create.go
Outdated
@@ -47,12 +46,12 @@ func Scaffold(path string) error { | |||
}, | |||
} | |||
|
|||
d, err := yaml.Marshal(m) | |||
d, err := json.Marshal(m) |
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.
One issue here is that the entire JSON file is written on a single line:
$ bat duffle.json
───────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
│ File: duffle.json
───────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
1 │ {"Name":"json-test","Version":"0.1.0","Description":"A short description of your bundle","Keywords":null,"Maintainers":null,"Components":{"cnab":{"Name":"cnab","Builder":"docker","Configura
│ tion":{"registry":"microsoft"}}},"Parameters":null,"Credentials":null}
───────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
With a bit of formatting, this would look great.
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.
Ugh... Good catch. json.MarshalIndent
to the rescue!
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.
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.
LGTM -- with a comment on formatting, that I think should be addressed before merging.
Signed-off-by: Matthew Fisher <[email protected]>
both comments addressed. Thanks guys! |
Signed-off-by: Matthew Fisher [email protected]