fix: instantiated pipeline vars #149
Conversation
jamiegosling
requested review from
odormond,
iAmoric,
SpectreVert,
aliculPix4D and
marco-m-pix4d
|
Thanks @jamiegosling! This actually shows me that I missed the details of the encoding of "instance vars". I appreciate that you found and expanded the existing tests. One thing to change is the Rummaging in the Concourse source code, I finally arrived at the source of truth, file https://github.com/concourse/concourse/blob/master/atc/pipeline.go. We will need to transplant some code and cover it with tests. If you want to iterate here, fine. Otherwise, I will take care of this (although I cannot guarantee a timeline). |
|
Thanks @marco-m-pix4d, I don't really know Go so if I get the time I'll iterate here as a learning experience, but if you fix it before then then that's great. Thanks for your time building this and looking at my issue! Jamie |
| @@ -345,13 +345,19 @@ func TestConcourseBuildURL(t *testing.T) { | |||
| name: "instanced vars 1", | |||
| env: testhelp.MergeStructs(baseEnv, | |||
| Environment{BuildPipelineInstanceVars: `{"branch":"stable"}`}), | |||
| want: "https://ci.example.com/teams/devs/pipelines/magritte/jobs/paint/builds/42?vars=%7B%22branch%22%3A%22stable%22%7D", | |||
| want: "https://ci.example.com/teams/devs/pipelines/magritte/jobs/paint/builds/42?vars.branch=%22stable%22", | |||
There was a problem hiding this comment.
@marco-m-pix4d should we start using go-vcr pkg in these tests?
I wonder if we could have catch'ed this sooner if we used it already.
There was a problem hiding this comment.
should we start using go-vcr pkg in these tests?
Probably.
I wonder if we could have catch'ed this sooner if we used it already.
This is the part I don't understand. I think that I tested against https://github.com/marco-m/concourse-in-a-box, so I don't understand why this now fails. True in any case that if we had a go-vcr trace, we could answer this question...
|
@jamiegosling see: #153 EDIT: I forgot to mention: you can already test my changes by using: |
|
@jamiegosling I am closing this PR as this was fixed in: #153 Thanks for your contribution and reporting the issue. |
Pipeline variables for instantiated pipelines should be encoded as a sequence of URL parameters in the format like
?vars.variableName1=variableValue1&vars.variableName2=variableValue2Closes #148