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

yaml merge key handling #11

Merged
merged 10 commits into from
Sep 16, 2023
Merged

yaml merge key handling #11

merged 10 commits into from
Sep 16, 2023

Conversation

clux
Copy link
Owner

@clux clux commented Sep 13, 2023

have it working under an optional flag that only works with yaml input.

not sure if this is enough to warrant a different input enum, or if it just needs another flag (that doesn't clash with jq) that only works in the yaml mode..

maybe we should just always do this for yaml tbh - it makes no sense to pass through tags to jq.

Signed-off-by: clux <sszynrae@gmail.com>
don't need to strip in profile if we do it in release job
besides, strip fails on mac with serde now
release cleanup: why are we putting readme + license in the archive?

Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Owner Author

clux commented Sep 15, 2023

three options here:

make this mandatory for yaml

PRO: always merge on yaml, less user confusion
CON:

  • not sure if there are errors this can cause on non YAML 1.2 users (apply_merge can fail)
  • people could genuinely use << as a key as it's not part of the standard

make it opt -in as a flag

PRO: two different ways to parse yaml, one cheap, one more expensive. we support both.
CON:

  • flag is yaml centric and must be tied to input

make it opt-in as an input format

PRO: more cleanly separates the the distinction, and no extra flag that only works with yaml needed
CON:

  • unclear what to name it (yaml vs YAML, or bring in yaml standard version names)
  • awkward binary usage yaml is implied by yq binary name and alias tq=yq -i=toml

kind of leaning to keeping it as an opt-in flag for now.

Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Owner Author

clux commented Sep 16, 2023

decided on -x for now to avoid taking -m which i feel could be useful if we want to backtrack on the multidoc default we have set

@clux
Copy link
Owner Author

clux commented Sep 16, 2023

ok tried three different variants on documents with merge and it's all weird AF if you don't apply merges (we are already singleton mapping so it's really odd to not apply merges).

here's what the circle test looks like with partial expanding (singleton but no apply_merge):

# yq -y --partial-expand .workflows test/circle.yaml
my_flow:
  jobs:
  - build:
      filters:
        <<:
          tags:
            only: /.*/
  - release:
      filters:
        <<:
          branches:
            ignore: /.*/
          tags:
            only: /v[0-9]+(\.[0-9]+)*/

which incidentally is also how it looks even without singleton mapping...

with both it looks like this:

$ yq -y .workflows test/circle.yml
my_flow:
  jobs:
  - build:
      filters:
        tags:
          only: /.*/
  - release:
      filters:
        branches:
          ignore: /.*/
        tags:
          only: /v[0-9]+(\.[0-9]+)*/

so have left full expansion the only sensible behaviour

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux linked an issue Sep 16, 2023 that may be closed by this pull request
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux marked this pull request as ready for review September 16, 2023 21:15
@clux clux changed the title yaml merge key support yaml merge key handling Sep 16, 2023
@clux clux merged commit 8ca95df into main Sep 16, 2023
13 checks passed
@clux clux deleted the merge-support branch September 16, 2023 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

not applying yaml merges
1 participant