Even More Pipeline Parsing Fixes #2253
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
moskyb
force-pushed
the
even-more-parsing-fixes
branch
from
8b5fe95
to
fef91e3
Compare
moskyb
requested review from
DrJosh9000 and
triarius
and removed request for
DrJosh9000
moskyb
changed the title
triarius
reviewed
Jul 31, 2023
.buildkite/steps/test-go-fmt.sh
Outdated
| @@ -10,6 +10,8 @@ fi | |||
|
|
|||
| if [[ $(go mod tidy -v 2> >(wc -c)) != 0 ]]; then # go mod tidy -v outputs to stderr for some reason | |||
| echo "The go.mod file is out of sync with the source code" | |||
| echo "go mod tidy had output:" | |||
| go mod tidy -v | |||
|
|
||
| case o.Contains("trigger"): | ||
| step = make(TriggerStep) | ||
| func stepFromMap(o *ordered.MapSA) (Step, error) { |
moskyb
force-pushed
the
even-more-parsing-fixes
branch
from
24a2aa6
to
46492d5
Compare
DrJosh9000
approved these changes
Jul 31, 2023
moskyb
force-pushed
the
even-more-parsing-fixes
branch
2 times, most recently
from
0b9acba
to
4d3a73b
Compare
moskyb
force-pushed
the
even-more-parsing-fixes
branch
from
4d3a73b
to
4746e6a
Compare
DrJosh9000
reviewed
Jul 31, 2023
internal/pipeline/step.go
Outdated
| @@ -29,6 +30,29 @@ type Step interface { | |||
| selfInterpolater | |||
| } | |||
|
|
|||
| type ScalarStep string | |||
There was a problem hiding this comment.
My feelings about the types here: I kinda want for it so that the types are easy to assemble in order to produce pipelines with Go. "ScalarStep" describes "a shape of data" more than "a type of step", and also overlaps semantic meaning with WaitStep and InputStep. But it's not too hard to use.
There was a problem hiding this comment.
yeah, i had this thought as well... the other idea i had was something like
type WaitStep struct {
Scalar string `yaml:"-"`
RemainingFields map[string]any `yaml:",inline"`
}and then have the marshal return the value of scalar if it's defined, and a regular marshal if not. is that something that feels better to you?
There was a problem hiding this comment.
Yeah sort of. I feel like if someone wants to write WaitStep{} it should marshal as "wait", because that's the kind of step it is. And then when we parse "wait" we should provide WaitStep{}, so that there's a canonical representation.
There was a problem hiding this comment.
this is updated in a9621be - would love your thoughts!
moskyb
force-pushed
the
even-more-parsing-fixes
branch
from
a3e261f
to
a9621be
Compare
DrJosh9000
approved these changes
Jul 31, 2023
Comment on lines
12
to
35
| // In the buildkite pipeline yaml, some step types (broadly, wait steps, input steps and block steps) can be represented | ||
| // either by a scalar string (ie "wait") or by a mapping with keys and values and such | ||
| // | ||
| // This behaviour is difficult to cleanly model in go, which leads to the somewhat odd structure of the structs defined | ||
| // in this file - each type (WaitStep, InputStep) has a Scalar field which is set to the scalar value if the step was | ||
| // if, during pipeline parsing, the step was represented as a scalar, and is left empty if the step was represented as | ||
| // a mapping. In essence, if one of the fields of these structs is filled, the other should be empty. | ||
| // | ||
| // In reading this file, you may have noticed that I mentioned that there are three types of step that can be represented | ||
| // as a scalar, but there are only two structs defined here. This is because the third type, block steps, are represented | ||
| // in exactly the same way as input steps, so they can share the same struct. This is liable to change in the future, | ||
| // as conceptually they're different types, and it makes sense to have them as different types in go as well. | ||
| // | ||
| // Also also! The implementations for WaitStep and InputStep **almost**, but not quite identical. This is due to the behaviour | ||
| // of marshalling an empty struct into into JSON. For WaitStep, it makes sense that the empty &WaitStep{} struct marshals | ||
| // to "wait", but with InputStep, there's no way to tell whether it should be marshalled to "input" or "block", which | ||
| // have very different behaviour on the backend. |
There was a problem hiding this comment.
Excellent commentary 👏👏
moskyb
force-pushed
the
even-more-parsing-fixes
branch
from
a9621be
to
de460b3
Compare
triarius
approved these changes
Jul 31, 2023
moskyb
force-pushed
the
even-more-parsing-fixes
branch
from
de460b3
to
07d96f1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR: Fixes two issues in the pipeline parser:
type: "block"). We should also support these steps.