search

Pix4D / cogito

Concourse resource for GitHub Commit Status and Google Chat notifications
MIT License
35 stars 14 forks source link

fix: instantiated pipeline vars #149

Closed jamiegosling closed 3 months ago

jamiegosling commented 5 months ago

Pipeline variables for instantiated pipelines should be encoded as a sequence of URL parameters in the format like

?vars.variableName1=variableValue1&vars.variableName2=variableValue2

Closes #148

marco-m-pix4d commented 5 months ago

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 panic, we do not panic in Cogito.

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).

jamiegosling commented 5 months ago

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

aliculPix4D commented 4 months ago

@jamiegosling see: https://github.com/Pix4D/cogito/pull/153

EDIT: I forgot to mention: you can already test my changes by using: pix4d/cogito:pci-3688-cogito-encode-url docker image https://hub.docker.com/layers/pix4d/cogito/pci-3688-cogito-encode-url/images/sha256-e9bbc1f3b516d95fff5c3e283c0e5d7c01570c2316cf17052631cf72682b8905?context=explore Can you please confirm that this solves your issue?

aliculPix4D commented 3 months ago

@jamiegosling I am closing this PR as this was fixed in: https://github.com/Pix4D/cogito/pull/153

Thanks for your contribution and reporting the issue.