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 task-output-json to work as documented #21

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

seanmil
Copy link
Contributor

@seanmil seanmil commented Sep 9, 2023

Currently the task_output_keys/values are always printed with double quotes around the value, preventing task-output-json from working as intended.

Additionally, this includes a refactoring of the unit test as the current one is flawed and does not test anything meaningful.

@seanmil seanmil requested a review from a team as a code owner September 9, 2023 21:02
@seanmil
Copy link
Contributor Author

seanmil commented Sep 9, 2023

Note, if the system running the unit tests does not have an iconv that supports the --unicode-subst option then this requires #20 to be merged first in order for all the tests to succeed properly.

@m0dular
Copy link
Contributor

m0dular commented Sep 11, 2023

There's an issue with the tests where the repo is cloned with the puppetlabs- prefix, so it's trying to source the script from

$PT__installdir/bash_task_helper

when it lives at $PT__installdir/puppetlabs-bash_task_helper in CI. I know that r10k will install the module without the puppetlabs- prefix so this seems like a CI problem, but I'm not sure what to change in order to fix this.

@seanmil
Copy link
Contributor Author

seanmil commented Sep 11, 2023

There's an issue with the tests where the repo is cloned with the puppetlabs- prefix, so it's trying to source the script from

$PT__installdir/bash_task_helper

when it lives at $PT__installdir/puppetlabs-bash_task_helper in CI. I know that r10k will install the module without the puppetlabs- prefix so this seems like a CI problem, but I'm not sure what to change in order to fix this.

I rebased my original commit and then modified the tests to avoid needing to know the name of the directory it was cloned as. I couldn't see a drawback to this approach and tests are passing now.

@m0dular
Copy link
Contributor

m0dular commented Sep 11, 2023

This should be fixed in #23 as well. Does your approach of source "$PT__installdir/files/task_helper.sh" work when actually running a task with Bolt that uses the helper? If so, then either approach would probably be ok.

@seanmil
Copy link
Contributor Author

seanmil commented Sep 11, 2023

This should be fixed in #23 as well. Does your approach of source "$PT__installdir/files/task_helper.sh" work when actually running a task with Bolt that uses the helper? If so, then either approach would probably be ok.

No, with my approach the "examples/mytask.sh" task would not work when run via Bolt. Let's go with your fix in #23 ! I've dropped my path change and repushed just the original change/fix.

@MartyEwings MartyEwings added the bug Something isn't working label Sep 12, 2023
@m0dular
Copy link
Contributor

m0dular commented Sep 12, 2023

I think we just need a rebase from main for this branch and it's good to go. The tests passed for me locally after doing so.

Currently the task_output_keys/values are always printed with
double quotes around the value, preventing task-output-json
from working as intended.

Additionally, this includes a refactoring of the unit test as
the current one is flawed and does not test anything meaningful.
Copy link
Contributor

@m0dular m0dular left a comment

Choose a reason for hiding this comment

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

👍

@m0dular m0dular merged commit 869188c into puppetlabs:main Sep 13, 2023
8 checks passed
@seanmil seanmil deleted the fix_json_encoding branch October 16, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants