Fix regression preventing the use of nested fields with fly set-pipeline --var
#6280
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
#6041 Adds a new KVPairs type that is a list of key-value pairs. KVPairs can be Expanded into a StaticVariables (which supports the usual Get/List methods of a Variables impl), and StaticVariables can be Flattened into a KVPairs. The intention of Expand is to go from a list of var flags to a StaticVariables for interpolating the pipeline template. The intention of Flatten is to convert a pipeline's instance vars to a flat string form using dot notation. Note that we already have an implementation of both methods in atc/pipeline.go for instance vars - however, I intend to use this simplified implementation that does not traverse lists since the notation for doing so is ambiguous (if a field name is an integer, it assumes that it's an index in a list). Instead of using dot notation for lists, we'll just display the list as '[a, b, c]' Signed-off-by: Aidan Oldershaw <[email protected]>
#6041 Previously, fly execute was using the VariablePairFlag to specify input mappings. Since we simply parsed key-value pairs as strings, this worked fine - but, as I intend to make the VariablePairFlag smarter about the keys (change them to vars.Reference), we'll need a separate flag for input mappings. I also added a helper to parse key-value pairs, since we have a whole bunch of flags that do similar things. This is technically a small behaviour change as it changes some of the error messages. Signed-off-by: Aidan Oldershaw <[email protected]>
I noticed a subtle bug in var parsing:
* ((vault:"my:var")) parses correctly, but
* (("my:var")) does not (the Source is set as `"my`, and the path as
`var`)
This commit fixes that bug by manually parsing the reference rather than
using a regular expression. It's easier to have fine control over the
parsing this way IMO.
I also made vars.ParseReference public - I was planning on doing so,
anyway, as I intend to use it outside of vars.Template (specifically,
for parsing variable flags)
Signed-off-by: Aidan Oldershaw <[email protected]>
#6041 ...and use the references to properly construct a StaticVariables. This also changes the YAMLVariableFlag slightly - rather than using "gopkg.in/yaml.v2" to parse the YAML value, it uses "sigs.k8s.io/yaml", which converts YAML to JSON first, and then unmarshals the JSON. This is the package we use in most of Concourse. The reason for the change is that parsing the YAML directly results in YAML objects being unmarshaled as `map[interface{}]interface{}`, which is less than ideal. However, it also means that numbers will be interpolated as `float64`, which is also less than ideal - so, instead, we unmarshal numbers as `json.Number`, which is just the string value of the number that's passed in. We could possibly get rid of the map[interface{}]interface{} handling in the vars package now, but I'm a bit worried about the implications of that, so I'll hold off for now. Signed-off-by: Aidan Oldershaw <[email protected]>
#6041 Disabling unquoting allows us to quote var refs to include literal '.' and ':'. For instance: fly sp -p test -c config.yml -v '"foo.bar"=baz' would give an error (invalid syntax) with unquoting enabled Signed-off-by: Aidan Oldershaw <[email protected]>
Signed-off-by: Aidan Oldershaw <[email protected]>
taylorsilva
reviewed
Nov 23, 2020
There was a problem hiding this comment.
Did a quick acceptance and looks good so far:
job job has changed:
name: job
plan:
- config:
image_resource:
source:
repository: busybox
type: registry-image
params:
- BAR: ((foo.bar))
- BAZ: ((foo.baz))
+ BAR: "123"
+ BAZ: "456"I compiled fly locally when doing my acceptance and also updated Dockerfile to use the release-67 tag for the dev image.
I'm going to go over the code now to double-check that nothing else got in during the cherry-pick
taylorsilva
approved these changes
Nov 24, 2020
|
Think I have to hit the "Update Branch" button again. It pops up every time the |
This pull request was closed.
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.
What does this PR accomplish?
Bug Fix | Feature | Documentation
closes #6279 .
cherry-picks #6170 to release/6.7.x (the non-instance vars stuff)
Changes proposed by this PR:
Notes to reviewer:
This was a bit of a doozy to cherry-pick since it built off of a couple refactors that I didn't include - hopefully I didn't break anything
Release Note
fly set-pipeline --varfly set-pipeline --var foo.bar=123 --var foo.baz=456would create the variablefoowith value{bar: 123, baz: 456}(that can be referenced in the pipeline config as((foo.bar)),((foo.baz)))"foo.bar" = 123and"foo.baz" = 456(that would have to be referenced in the pipeline config as(("foo.bar"))and(("foo.baz")), respectively).in it, you can now quote the flag: e.g.fly set-pipeline --var '"foo.bar"=123'(note that this requires quoting the entire flag in'...'to avoid shell expansion)Contributor Checklist
Reviewer Checklist
BOSH and
Helm packaging; otherwise, ignored for
the integration
tests
(for example, if they are Garden configs that are not displayed in the
--helptext).