Allow arguments to be passed to npm scripts #356

oltionchampari · 2020-04-16T13:37:03Z

Closes #354

Developer Checklist (Definition of Done)

Summary of changes

Appended -- to npm scripts.
Tested the steps as described in Append '--' to the created npm commands in workspace package.json #354 and this fix works as expected.
Added -- to the server scripts, i am not sure that is necessary but it has no effect on the functionality.

To test

Add a script to a plugin i.e., "myscript":"git"
yo phovea:update in workspace
npm run myscript:<plugin> -- -b my_branch

#354

thinkh

Please test the withinEnv case again and post the results here. Thanks.

thinkh · 2020-05-04T12:56:50Z

generators/workspace/index.js

@@ -265,7 +265,7 @@ class Generator extends Base {
        // no pre post test tasks
        cmds.filter((s) => toPatch.test(s)).forEach((s) => {
          // generate scoped tasks
-          let cmd = `.${path.sep}withinEnv "exec 'cd ${p} && npm run ${s}'"`;
+          let cmd = `.${path.sep}withinEnv "exec 'cd ${p} && npm run ${s}'" --`;


Suggested change

let cmd = `.${path.sep}withinEnv "exec 'cd ${p} && npm run ${s}'" --`;

let cmd = `.${path.sep}withinEnv "exec 'cd ${p} && npm run ${s} --'"`;

@oltionchampari Did you test this case? @anita-steiner and I assume that the -- must be placed directly after the run command (as in the other change above). It could even be that this does not work at all, as you want to pass arguments through multiple layers (e.g., withinEnv and exec).

Running npm run check:phovea_server or any other withinEnv command from the workspace results in an error.

I do not know if there is a problem with my docker setup or the withinEnv script is outdated.
@thinkh Could you please test if the commands work for you. Thanks!

You are right. This whole command does not work...

Investigation report

(I had to switch the line ending for withinEnv from CRLF to LF, which might be caused by my Windows)

I can confirm your results; I get the same output

Executing the command from withinEnv manually (docker-compose run --service-ports api) starts the container

docker-compose run --service-ports api exec results in Error response from daemon: OCI runtime create failed: container_linux.go:349: starting container process caused "exec: \"exec\": executable file not found in $PATH": unknown

Checking the docker documentation it can be either docker-compose run OR docker-compose exec

There is no such command docker-compose run ... exec

docker-compose run --service-ports api /bin/sh results in a terminal inside the container

However, npm -v fails with /bin/sh: 2: npm: not found, as npm is not part of the python:3.7 base image!

docker-compose run --service-ports api /bin/sh -c "cd phovea_server && npm run test" should be the correct command, but it also fails with /bin/sh: 1: npm: not found (see previous point)

Replacing docker-compose run --service-ports api with withinEnv (command: ./withinEnv /bin/sh -c "cd phovea_server && echo test") prints docker-compose run --service-ports api /bin/sh -c cd phovea_server && echo test, i.e., without additional quotes and results in an empty terminal output

Escaping the command ./withinEnv "/bin/sh -c \"cd phovea_server && echo test\"" results in docker-compose run --service-ports api /bin/sh -c "cd phovea_server && echo test" and prints phovea_server: 1: phovea_server: Syntax error: Unterminated quoted string

Since the print statement itself works, but the execution itself fails it must be a problem with the $@

Reading the documentation about $@ it seems that it is not possible to pass strings from incoming parameter to the subsequent command

An alternative syntax to -c is the <<< syntax (see https://stackoverflow.com/a/40487106)

./withinEnv /bin/bash <<< "cd phovea_server && echo test" results in the expected output test

Note that the whole bin/sh command is not wrapped in single quotes so that $@ can expand it as different paramters and pass it down to docker-compose

Copying the new command to package.json ("test:phovea_server": "./withinEnv /bin/bash <<< \"cd phovea_server && npm run test\"") and executing npm run test:phovea_server results in the following error: sh: 1: Syntax error: redirection unexpected

I found no obvious way how to fix this syntax in the npm script

The only solution I found is to move the redirection into the withinEnv: docker-compose run --service-ports api /bin/bash <<< $@

Change the npm script to ./withinEnv \"cd phovea_server && npm run test\"

Running npm run test:phovea_server results in the expected /bin/sh: 1: npm: not found

Now only the npm script npm run start:phovea_server does not work anymore and needs to be changed to "start:phovea_server": "./withinEnv \"cd phovea_server && npm run start\"",

Alternatively we can use "start:phovea_server": "./withinEnv \"python phovea_server\"", to start the phovea server

#356

thinkh

See investigation report for the full list of commands I tried.

TL;DR

Long story short... this command will never work, since npm is not part of the python:3.7 base image.

Beside that fact the exec call needs to be removed and the quotes needs to be changed from the npm scripts:

    "start:phovea_server": "./withinEnv \"cd phovea_server && npm run start\"",
    "check:phovea_server": "./withinEnv \"cd phovea_server && npm run check\"",
    "test:phovea_server": "./withinEnv \"cd phovea_server && npm run test\"",
    "dist:phovea_server": "./withinEnv \"cd phovea_server && npm run dist\""

Last the withinEnv needs to be adapted by the /bin/sh <<< call:

#!/usr/bin/env bash
set -e

echo "docker-compose run --service-ports api /bin/bash <<< $@"
docker-compose run --service-ports api /bin/bash <<< $@

Finally, I'm not really sure, if we need the -- at all since all parameters are passed $@ to the bash. I couldn't test it with the npm script either since npm is not installed.

thinkh · 2020-06-12T14:35:53Z

generators/workspace/index.js

@@ -265,7 +265,7 @@ class Generator extends Base {
        // no pre post test tasks
        cmds.filter((s) => toPatch.test(s)).forEach((s) => {
          // generate scoped tasks
-          let cmd = `.${path.sep}withinEnv "exec 'cd ${p} && npm run ${s}'"`;
+          let cmd = `.${path.sep}withinEnv "exec 'cd ${p} && npm run ${s}'" --`;


You are right. This whole command does not work...

Investigation report

(I had to switch the line ending for withinEnv from CRLF to LF, which might be caused by my Windows)

I can confirm your results; I get the same output

Executing the command from withinEnv manually (docker-compose run --service-ports api) starts the container

docker-compose run --service-ports api exec results in Error response from daemon: OCI runtime create failed: container_linux.go:349: starting container process caused "exec: \"exec\": executable file not found in $PATH": unknown

Checking the docker documentation it can be either docker-compose run OR docker-compose exec

There is no such command docker-compose run ... exec

docker-compose run --service-ports api /bin/sh results in a terminal inside the container

However, npm -v fails with /bin/sh: 2: npm: not found, as npm is not part of the python:3.7 base image!

docker-compose run --service-ports api /bin/sh -c "cd phovea_server && npm run test" should be the correct command, but it also fails with /bin/sh: 1: npm: not found (see previous point)

Replacing docker-compose run --service-ports api with withinEnv (command: ./withinEnv /bin/sh -c "cd phovea_server && echo test") prints docker-compose run --service-ports api /bin/sh -c cd phovea_server && echo test, i.e., without additional quotes and results in an empty terminal output

Escaping the command ./withinEnv "/bin/sh -c \"cd phovea_server && echo test\"" results in docker-compose run --service-ports api /bin/sh -c "cd phovea_server && echo test" and prints phovea_server: 1: phovea_server: Syntax error: Unterminated quoted string

Since the print statement itself works, but the execution itself fails it must be a problem with the $@

Reading the documentation about $@ it seems that it is not possible to pass strings from incoming parameter to the subsequent command

An alternative syntax to -c is the <<< syntax (see https://stackoverflow.com/a/40487106)

./withinEnv /bin/bash <<< "cd phovea_server && echo test" results in the expected output test

Note that the whole bin/sh command is not wrapped in single quotes so that $@ can expand it as different paramters and pass it down to docker-compose

Copying the new command to package.json ("test:phovea_server": "./withinEnv /bin/bash <<< \"cd phovea_server && npm run test\"") and executing npm run test:phovea_server results in the following error: sh: 1: Syntax error: redirection unexpected

I found no obvious way how to fix this syntax in the npm script

The only solution I found is to move the redirection into the withinEnv: docker-compose run --service-ports api /bin/bash <<< $@

Change the npm script to ./withinEnv \"cd phovea_server && npm run test\"

Running npm run test:phovea_server results in the expected /bin/sh: 1: npm: not found

Now only the npm script npm run start:phovea_server does not work anymore and needs to be changed to "start:phovea_server": "./withinEnv \"cd phovea_server && npm run start\"",

Alternatively we can use "start:phovea_server": "./withinEnv \"python phovea_server\"", to start the phovea server

Append -- to workspace script

7de454b

#354

oltionchampari added type: feature New feature or request release: minor PR merge results in a new minor version labels Apr 16, 2020

oltionchampari requested a review from a user April 16, 2020 13:37

oltionchampari requested a review from thinkh as a code owner April 16, 2020 13:37

oltionchampari assigned ghost Apr 16, 2020

dvvanessastoiber mentioned this pull request Apr 20, 2020

Release 5.1.0 #359

Merged

21 tasks

thinkh requested changes May 4, 2020

View reviewed changes

oltionchampari added a commit that referenced this pull request May 6, 2020

Testing

d5ec555

#356

thinkh requested changes Jun 12, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow arguments to be passed to npm scripts #356

Allow arguments to be passed to npm scripts #356

oltionchampari commented Apr 16, 2020 •

edited

Loading

thinkh left a comment

thinkh May 4, 2020

oltionchampari May 14, 2020

thinkh Jun 12, 2020

thinkh left a comment

thinkh Jun 12, 2020

	let cmd = `.${path.sep}withinEnv "exec 'cd ${p} && npm run ${s}'" --`;
	let cmd = `.${path.sep}withinEnv "exec 'cd ${p} && npm run ${s} --'"`;

Allow arguments to be passed to npm scripts #356

Are you sure you want to change the base?

Allow arguments to be passed to npm scripts #356

Conversation

oltionchampari commented Apr 16, 2020 • edited Loading

Developer Checklist (Definition of Done)

Summary of changes

To test

thinkh left a comment

Choose a reason for hiding this comment

thinkh May 4, 2020

Choose a reason for hiding this comment

oltionchampari May 14, 2020

Choose a reason for hiding this comment

thinkh Jun 12, 2020

Choose a reason for hiding this comment

Investigation report

thinkh left a comment

Choose a reason for hiding this comment

thinkh Jun 12, 2020

Choose a reason for hiding this comment

Investigation report

oltionchampari commented Apr 16, 2020 •

edited

Loading