Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(schema): stages don't require name field #105

Merged
merged 2 commits into from
Aug 24, 2020
Merged

Conversation

wass3r
Copy link
Collaborator

@wass3r wass3r commented Aug 22, 2020

name is set to key if not supplied as evidenced by: https://github.com/go-vela/types/blob/master/yaml/stage.go#L78

@wass3r wass3r requested a review from a team as a code owner August 22, 2020 03:09
@wass3r wass3r self-assigned this Aug 22, 2020
@wass3r wass3r added the bug Indicates a bug label Aug 22, 2020
@codecov
Copy link

codecov bot commented Aug 22, 2020

Codecov Report

Merging #105 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #105   +/-   ##
=======================================
  Coverage   99.67%   99.67%           
=======================================
  Files          44       44           
  Lines        3096     3096           
=======================================
  Hits         3086     3086           
  Misses          6        6           
  Partials        4        4           
Impacted Files Coverage Δ
yaml/stage.go 100.00% <ø> (ø)

@kneal
Copy link

kneal commented Aug 22, 2020

Do you have an example of where that’s true? I thought that field was required since the compiler has some dependencies on it. https://github.com/go-vela/compiler/blob/master/compiler/native/transform.go#L20

@wass3r
Copy link
Collaborator Author

wass3r commented Aug 23, 2020

not sure i understand. based on our example in the docs: https://go-vela.github.io/docs/concepts/pipeline/stages/#syntax and my own usage, it's not required to supply name for your stage. the key will be used if no stage name was supplied, as long as we use the unmarshall function on the struct, which it should every time we use yaml.Unmarshal. maybe i'm missing something.

Copy link
Contributor

@jbrockopp jbrockopp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kneal
Copy link

kneal commented Aug 24, 2020

@wass3r Yeah, you're right looks like it gets implicitly set
https://github.com/go-vela/types/blob/master/yaml/stage.go#L76-L79

Copy link

@kneal kneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐬

@kneal kneal merged commit 3e7f85b into master Aug 24, 2020
@kneal kneal deleted the fix/schema-stages branch August 24, 2020 13:26
@wass3r
Copy link
Collaborator Author

wass3r commented Aug 24, 2020

Cool, yea.. we require it internally, but from a user's perspective it's optional. sorry for confusion.

@kneal
Copy link

kneal commented Aug 24, 2020

No problem! I just remembered seeing the name: be used in areas of the compiler and didn’t think to look if we implicitly set it when it’s missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants