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

fix: Handle multiline strings in yaml serialization. #935

Merged
merged 4 commits into from
Jan 22, 2024

Conversation

DanCardin
Copy link
Contributor

Pull Request Checklist

Description of PR
Currently, multiline strings serialize to yaml in a way that is not true to the original content.

For example, copying argo workflows' exit-handler example

excerpt:

      args: [
        "curl -X POST --data-urlencode 'payload={
          \"text\": \"{{workflow.name}} finished\",
          \"blocks\": [
            {
              \"type\": \"section\",
              \"text\": {
                \"type\": \"mrkdwn\",
                \"text\": \"Workflow {{workflow.name}} {{workflow.status}}\",
              }
            }
          ]
        }'
        YOUR_WEBHOOK_URL_HERE"
      ]

currently produces:

        args:
        - "curl -X POST --data-urlencode 'payload={\n          \"text\": \"{{workflow.name}}\
          \ finished\",\n          \"blocks\": [\n            {\n              \"\
          type\": \"section\",\n              \"text\": {\n                \"type\"\
          : \"mrkdwn\",\n                \"text\": \"Workflow {{workflow.name}} {{workflow.status}}\"\
          ,\n              }\n            }\n          ]\n        }'\n        YOUR_WEBHOOK_URL_HERE\"\
          \n        "

whereas this PR produces:

        args:
        - |-
          curl -X POST --data-urlencode 'payload={
                    "text": "{{workflow.name}} finished",
                    "blocks": [
                      {
                        "type": "section",
                        "text": {
                          "type": "mrkdwn",
                          "text": "Workflow {{workflow.name}} {{workflow.status}}",
                        }
                      }
                    ]
                  }'
                  YOUR_WEBHOOK_URL_HERE";

Note that this does not fix yaml/pyyaml#121 (comment). That is, the following example will revert back to original formatting due to the trailing \n at the end of the string.

args="""
two
"""

I would suggest switching ruamel.yaml over pyyaml just generally, which would fix this issue, but that's probably going to be considered a breaking change.

In any case is probably more tangential to the root issue above (which affects both libraries in the same way).

Copy link

codecov bot commented Jan 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4672a9b) 80.6% compared to head (1e82d34) 80.7%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #935     +/-   ##
=======================================
+ Coverage   80.6%   80.7%   +0.1%     
=======================================
  Files         49      50      +1     
  Lines       3898    3908     +10     
  Branches     792     793      +1     
=======================================
+ Hits        3145    3157     +12     
+ Misses       565     564      -1     
+ Partials     188     187      -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@flaviuvadan
Copy link
Collaborator

@DanCardin I think if you run a make regenerate-test-data this should regenerate all the docs and make the script field more readable for all of Hera's examples!

Signed-off-by: DanCardin <ddcardin@gmail.com>
Signed-off-by: DanCardin <ddcardin@gmail.com>
@DanCardin
Copy link
Contributor Author

DanCardin commented Jan 18, 2024

😱 neat! An even better way of exemplifying the benefit than my test, lol. Added as a 2nd commit, separate from the actual code change.

Just to check, are the literal examples that are being rewritten tested somehow to make sure this hasn't secretly broken the output somehow, even if it looks prettier?

@flaviuvadan
Copy link
Collaborator

flaviuvadan commented Jan 18, 2024

😱 neat! An even better way of exemplifying the benefit than my test, lol. Added as a 2nd commit, separate from the actual code change.

Just to check, are the literal examples that are being rewritten tested somehow to make sure this hasn't secretly broken the output somehow, even if it looks prettier?

Yea, they're all tested here! It essentially compares the YAML output but without formatting concerns. As long as the content is valid, the tests pass.

Note that we also have Argo Workflows running in CI via k3d and we submit a few workflows to it. Those are created via the some to_yaml API, so if those work, then we know that:

  • Hera serialization results in valid upstream examples
  • examples are actually submittable / accepted by the Argo server + runnable by the controller

@flaviuvadan flaviuvadan added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels Jan 18, 2024
Copy link
Collaborator

@flaviuvadan flaviuvadan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All those examples look sooo much better now 🎖️

@flaviuvadan
Copy link
Collaborator

We can 🚢 once @elliotgunton or @samj1912 approve too. Their stake in the CLI + YAML is larger than mine, so I want to be mindful of their input

src/hera/_yaml.py Outdated Show resolved Hide resolved
DanCardin and others added 2 commits January 19, 2024 15:12
Co-authored-by: Elliot Gunton <egunton@bloomberg.net>
Signed-off-by: DanCardin <DanCardin@users.noreply.github.com>
Copy link
Collaborator

@elliotgunton elliotgunton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff! 🚀 The dodgy source dumping has been bugging me forever! 😆

@elliotgunton elliotgunton merged commit 571a133 into argoproj-labs:main Jan 22, 2024
20 checks passed
@DanCardin DanCardin deleted the dc/multiline-yaml branch January 22, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why does pyyaml disallow trailing spaces in block scalars?
3 participants