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

refactor pipe script and improve debug mode #201

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

lukdz
Copy link
Contributor

@lukdz lukdz commented Aug 8, 2023

Description

  • switch to long options to make command more readable
  • improve debug mode by using set -x, which does show how command arguments are split;
    echo outputs values without quotes, which makes it difficult to debug arguments split by incorrectly escaped whitespaces
  • fix indents

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@npeltier
Copy link
Contributor

npeltier commented Aug 8, 2023

can you please show examples of what the output is?

@npeltier
Copy link
Contributor

npeltier commented Aug 8, 2023

(and good morning, thanks a lot for that contribution :p i forgot to be polite!)

@lukdz
Copy link
Contributor Author

lukdz commented Aug 8, 2023

Hi,

Example of execution of pipe with identical arguments but different debug implementation:

echo doesn't show where pipe_cmd argument starts and ends;
user doesn't know if /jcr:root... is interpreted as a part of pipe_cmd or the next curl argument
image

set -x with single quotes show where pipe_cmd argument starts and ends;
user does know that /jcr:root... is interpreted as a part of pipe_cmd
image

@lukdz
Copy link
Contributor Author

lukdz commented Aug 22, 2023

bump

Copy link
Contributor

@npeltier npeltier left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for your contribution! probably we should remove password from debug output though

@npeltier npeltier merged commit df881d5 into adobe:master Aug 22, 2023
2 checks passed
@lukdz
Copy link
Contributor Author

lukdz commented Aug 22, 2023

Thank you for approval.

b) opts="-F bindings='`generate_json "$2"`' $opts"; shift ;;
v) verbose="--verbose"; shift ;;
d) opts="--form dryRun=true $opts";;
b) opts="--form bindings='$(generate_json "$2")' $opts"; shift ;;

Choose a reason for hiding this comment

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

@lukdz I observe that the bindings to the content script are failing with the latest pull.
For example:
./pipe test.txt -n 1000 -b modifiedDate=2023-08-30T10:20:20.206Z

The script generates curl command like this for the binding and the script at AEM is failing with binding parameter not availalble:
-form ‘bindings=‘\’‘{“modifiedDate”:“2023-08-30T10:20:20.206Z”}‘\’’'

cc: @npeltier

Copy link
Contributor Author

@lukdz lukdz Sep 1, 2023

Choose a reason for hiding this comment

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

I tested example providede:

$ ./pipe test.txt -n 1000 -b modifiedDate=2023-08-30T10:20:20.206Z
+ curl --user admin:admin --show-error --silent --fail --form pipe_cmd=test.txt --form 'bindings='\''{"modifiedDate":"2023-08-30T10:20:20.206Z"}'\''' --form size=1000 http://localhost:4502/apps/dx/scripts/exec.json

Reverting:

-                b)  opts="--form bindings='$(generate_json "$2")' $opts"; shift ;;
+                b)  opts="-F bindings='`generate_json "$2"`' $opts"; shift ;;

Doesn't change the result:

$ ./pipe test.txt -n 1000 -b modifiedDate=2023-08-30T10:20:20.206Z
+ curl --user admin:admin --show-error --silent --fail --form pipe_cmd=test.txt -F 'bindings='\''{"modifiedDate":"2023-08-30T10:20:20.206Z"}'\''' --form size=1000 http://localhost:4502/apps/dx/scripts/exec.json

But adding back eval:

-  curl \
+ eval curl \

Does change the result:

$ ./pipe test.txt -n 1000 -b modifiedDate=2023-08-30T10:20:20.206Z
+ eval curl --user admin:admin --show-error --silent --fail --form pipe_cmd=test.txt -F 'bindings='\''{"modifiedDate":"2023-08-30T10:20:20.206Z"}'\''' --form size=1000 http://localhost:4502/apps/dx/scripts/exec.json
++ curl --user admin:admin --show-error --silent --fail --form pipe_cmd=test.txt -F 'bindings={"modifiedDate":"2023-08-30T10:20:20.206Z"}' --form size=1000 http://localhost:4502/apps/dx/scripts/exec.json

Looks like eval is removing '\''.
The proper fix seems to be removal of single quotes:

-                b)  opts="--form bindings='$(generate_json "$2")' $opts"; shift ;;
+                b)  opts="--form bindings=$(generate_json "$2") $opts"; shift ;;

Which results in (without adding back eval):

$ ./pipe test.txt -n 1000 -b modifiedDate=2023-08-30T10:20:20.206Z
+ curl --user admin:admin --show-error --silent --fail --form pipe_cmd=test.txt --form 'bindings={"modifiedDate":"2023-08-30T10:20:20.206Z"}' --form size=1000 http://localhost:4502/apps/dx/scripts/exec.json

This fix would require validation from @praveenrv
afterwards, it can be merged: #202

Choose a reason for hiding this comment

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

@lukdz thanks for the details and fix. I see it working after removing the single quotes.

opts="--form bindings=$(generate_json "$2") $opts"; shift ;;

test:

./pipe test.txt -n 1000 -b modifiedDate=2023-08-30T10:20:20.206Z
+ curl --user admin:admin --show-error --silent --fail --form [email protected] --form 'bindings={"modifiedDate":"2023-08-30T10:20:20.206Z"}' --form size=1000 http://localhost:4502/apps/dx/scripts/exec.json

cc: @npeltier 

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.

4 participants