actions / runner-container-hooks

Runner Container Hooks for GitHub Actions
MIT License
67 stars 43 forks source link

docker: Consider the string quoted for the argument passed as options in service container #84

Closed s4ichi closed 9 months ago

s4ichi commented 1 year ago

When specifying a string like the following in options of the service container, the container fails to start with an "invalid reference format" error. This PR addresses that issue with minimal changes.

db:
  image: mysql:8.0
  ports:
    - 3306:3306
  env:
    MYSQL_ALLOW_EMPTY_PASSWORD: 1
  options: >-
    --health-cmd "mysqladmin ping"
    --health-interval 10s
    --health-timeout 5s
    --health-retries 5

In the main branch, additional arguments received in options are split using split(' '):

In other words, the example above is interpreted as follows (pay attention to the content of the health-cmd argument):

['--health-cmd', '"mysqladmin', 'ping"', '--health-interval', '10s', '--health-timeout', '5s', '--health-retries', '5']

This splitting method is invalid as arguments passed to the command executed with @actions/exec. Therefore, when executing docker create with this options, it results in an invalid reference format error.

Regarding this issue, I consider it challenging to parse the options correctly while maintaining compatibility. Therefore, I have made a change not to split string values that are enclosed in " or '. As a result of this modification, the previous example is interpreted as follows.

['--health-cmd', 'mysqladmin ping', '--health-interval', '10s', '--health-timeout', '5s', '--health-retries', '5']

With this change, the service container is successfully started.

nikola-jokic commented 1 year ago

Hey @s4ichi,

Thanks for pointing this out! I would love to avoid regexp since it is harder to read. We can maybe use a minimist package to parse arguments but I'll get back to you on that one :relaxed:

fhammerl commented 9 months ago

Hey @s4ichi , thanks for making this PR. We're looking into lightly adjusting how we pass these arguments. The PR above should address the original issue that prompted this PR.