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

Multiline syntax with pipe (|) not retained #185

Open
tamird opened this issue Jun 1, 2024 · 13 comments
Open

Multiline syntax with pipe (|) not retained #185

tamird opened this issue Jun 1, 2024 · 13 comments
Labels
yaml_v3_problem A bug in the underlying yaml library. These issues are vastly harder to fix.

Comments

@tamird
Copy link

tamird commented Jun 1, 2024

I see this diff:

     steps:
       - name: Start PostgreSQL
         # We start PostgreSQL as a docker step instead of the GH `services` because it allows us to pass in custom flags (and seems to be faster to spin up?)
-        run: |
-          set -euo pipefail
-          args=(
-            -d
-            --name postgres 
-            --env POSTGRES_USER=postgres 
-            --env POSTGRES_PASSWORD=postgres 
-            --publish 5432:5432 
-            --health-cmd pg_isready 
-            --health-interval 10s 
-            --health-timeout 5s 
-            --health-retries 5 
-            public.ecr.aws/docker/library/postgres:16-alpine 
-
-            # This is to allow many connections from xdist - we run as many DBs in parallel as CPUs on the box
-            -c max_locks_per_transaction=1000 
-            -c max_connections=1000
-          )
-          docker run "${args[@]}"
-
+        run: "set -euo pipefail\nargs=(\n  -d\n  --name postgres \n  --env POSTGRES_USER=postgres \n  --env POSTGRES_PASSWORD=postgres \n  --publish 5432:5432 \n  --health-cmd pg_isready \n  --health-interval 10s \n  --health-timeout 5s \n  --health-retries 5 \n  public.ecr.aws/docker/library/postgres:16-alpine \n      #magic___^_^___line\n  # This is to allow many connections from xdist - we run as many DBs in parallel as CPUs on the box\n  -c max_locks_per_transaction=1000 \n  -c max_connections=1000\n)\ndocker run \"${args[@]}\"\n      #magic___^_^___line\n"

This was previously discussed in #62 (comment).

@braydonk
Copy link
Collaborator

braydonk commented Jun 3, 2024

Hi @tamird thanks for opening an issue. Not sure whether this will work, but does adding scan_folded_as_literal: true fix this?

@braydonk braydonk added the yaml_v3_problem A bug in the underlying yaml library. These issues are vastly harder to fix. label Jun 3, 2024
@tamird
Copy link
Author

tamird commented Jun 3, 2024

No, the behavior is unchanged.

@kachick

This comment was marked as resolved.

@seastco
Copy link

seastco commented Jun 11, 2024

seeing this as well

@19ngaddam
Copy link

19ngaddam commented Jun 11, 2024

the "fix" that worked for me was ensuring that there were no trailing spaces anywhere in the multiline block (including after the |). yamlfmt was able to preserve the format in that case. Ideally, there should be an option to ignore the format for multiline literal style strings.

@braydonk
Copy link
Collaborator

Right I remember this now. I was trying to solve this in the yaml library and got stuck.

An interim fix could be to add a feature that trims the whitespace at the end of every string; that's doable without parsing. It would probably be a relatively useful feature anyway.

My time to work on this project over the next couple weeks is unfortunately quite limited so I can't give an estimate for when that will get done, but next time I have time I will get this done.

@thiagowfx
Copy link
Contributor

Just noting that something similar happens with >- (it's likely the same root cause).

To add another future test case idea:

Observed:

diff --git ansible/acme.sh/acme.yml ansible/acme.sh/acme.yml
index 08b5f893..1c9549be 100644
--- ansible/acme.sh/acme.yml
+++ ansible/acme.sh/acme.yml
@@ -44,12 +44,7 @@

 - name: Issue cert for domain via DNS challenge mode
   command: >-
-    ./acme.sh --issue -d {{ acme_sh_domain }}
-    --challenge-alias {{ acme_sh_challenge_domain }}
-    --dns {{ acme_sh_dns_provider }}
-    --accountemail {{ acme_sh_mail }}
-    {{ "--staging" if acme_sh_use_staging is defined  else "" }}
-    {{ "--debug" if acme_sh_debug is defined else "" }}
+    ./acme.sh --issue -d {{ acme_sh_domain }} --challenge-alias {{ acme_sh_challenge_domain }} --dns {{ acme_sh_dns_provider }} --accountemail {{ acme_sh_mail }} {{ "--staging" if acme_sh_use_staging is defined  else "" }} {{ "--debug" if acme_sh_debug is defined else "" }}
   args:
     chdir: "~/.acme.sh"
   environment:

Desired / expected: Newlines should have been preserved.

@braydonk
Copy link
Collaborator

I did add trim_trailing_whitespace as an option to the formatter. With this turned on, this bug shouldn't get tripped since it's caused by trailing whitespace. Not a perfect fix, but a stopgap at least.

@nikaro
Copy link
Contributor

nikaro commented Jul 29, 2024

I did add trim_trailing_whitespace as an option to the formatter. With this turned on, this bug shouldn't get tripped since it's caused by trailing whitespace. Not a perfect fix, but a stopgap at least.

This does not work for with >, only with |.

# cat .yamlfmt.yaml
trim_trailing_whitespace: true
# cat hello.yaml
multiline_without_trailing_whitspaces: >-
  hello
  world
# yamlfmt -debug=all -dry hello.yaml
[DEBUG]: No config path specified in -conf
[DEBUG]: Found config at /var/folders/pz/wn2sts951hbdjx0nzdr1c_h40000gp/T/tmp.J6M98dAOmk/.yamlfmt.yaml
[DEBUG]: Found config at /var/folders/pz/wn2sts951hbdjx0nzdr1c_h40000gp/T/tmp.J6M98dAOmk/.yamlfmt.yaml
[DEBUG]: using file path matching. include patterns: [hello.yaml]
[DEBUG]: found paths: [hello.yaml]
[DEBUG]: paths to format: map[hello.yaml:{}]
hello.yaml:
  multiline_without_trailing_whitspaces: >-  multiline_without_trailing_whitspaces: >-
-   hello                                      hello world
-   world

@braydonk
Copy link
Collaborator

braydonk commented Jul 29, 2024

Hi @nikaro,

Two things:

  1. Is the example in cat .yamlfmt.yaml your exact config file? If so, that will not work. trim_trailing_whitespace goes under a formatter.
  2. Folded scalar style (>) are a separate issue. For those, you will also need to set the formatter option scan_folded_as_literal: true. This is because when the underlying YAML parser reads the YAML and reproduces it, it tries to respect the > (which means to fold all newlines and produce the string as a single line). That option circumvents that behaviour.

Applying both of those suggestions, I expect it will start to work. If it doesn't, please open a new issue with similar reproduction steps.

EDIT: I corrected my suggestion in 2

@nikaro
Copy link
Contributor

nikaro commented Jul 29, 2024

Hi @braydonk

  1. No it wasn't my exact config, I tried to reproduce a minimal config/environment, my bad.
  2. Ah, I didn't knew this bit.

It works with your suggestion. Thanks.

@powerman
Copy link

I'm not sure is it the same issue or not, please let me know if it's better to open a new issue for this.

The problem is I'm unable to control "block chomping". No matter what I've inside a block and which config options I use it always replaces | and |+ with |-, and replaces > and >+ with >-. Is there any workaround for this?

@powerman
Copy link

Oops, sorry, actually modification of "block chomping" symbol happens because of both some config options and block content.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
yaml_v3_problem A bug in the underlying yaml library. These issues are vastly harder to fix.
Projects
None yet
Development

No branches or pull requests

8 participants