Skip to content

Docker and K8s: Fix shell arguments when split by the runner#115

Merged
nikola-jokic merged 2 commits intomainfrom
nikola-jokic/shell-parse
Nov 20, 2023
Merged

Docker and K8s: Fix shell arguments when split by the runner#115
nikola-jokic merged 2 commits intomainfrom
nikola-jokic/shell-parse

Conversation

@nikola-jokic
Copy link
Copy Markdown
Collaborator

@nikola-jokic nikola-jokic commented Nov 9, 2023

When runner passes arguments to the hook, there may be situations where quoted strings are interpreted as separate arguments.

This PR addresses the issue and fixes the arguments based on shlex expansion

Fixes the issue in this discussion

@nikola-jokic nikola-jokic requested review from a team as code owners November 9, 2023 11:42
@fhammerl fhammerl requested review from a team and removed request for a team November 9, 2023 13:17
}

export function fixArgs(args: string[]): string[] {
return shlex.split(args.join(' '))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is powershell ok with this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If arguments are not POSIX complaint, adding quotes around them would fix the problem

@nikola-jokic nikola-jokic force-pushed the nikola-jokic/shell-parse branch from cc019aa to 887f455 Compare November 16, 2023 13:51
@nikola-jokic nikola-jokic changed the title Docker: Fix shell arguments when split by the runner Docker and K8s: Fix shell arguments when split by the runner Nov 16, 2023
@timmjd
Copy link
Copy Markdown

timmjd commented Nov 16, 2023

@nikola-jokic : does this cover the same as described in #117?

@nikola-jokic
Copy link
Copy Markdown
Collaborator Author

Hey @timmjd,

Yes! Your solution helped fix one problem in case the quotes are broken. This is the problem I was fixing for docker, I just extended it to be the same for k8s. Thank you for raising it, by the way!
Would you mind testing it to make sure it works?

@timmjd
Copy link
Copy Markdown

timmjd commented Nov 17, 2023

Awesome!

This implementation is even better than #117 due to you keep the array consistency.

Example 1

name: Build MKDocs

runs:
  using: docker
  image: docker://squidfunk/mkdocs-material:latest
  entrypoint: mkdocs
  args:
  - build --verbose --strict
spec:
  containers:
  - command:
    - mkdocs
    args:
    - build --verbose --strict

Example 2

name: Build MKDocs

runs:
  using: docker
  image: docker://squidfunk/mkdocs-material:latest
  entrypoint: mkdocs
  args:
  - build
  - --verbose
  - --strict
spec:
  containers:
  - command:
    - mkdocs
    args:
    - build
    - --verbose
    - --strict

Copy link
Copy Markdown
Contributor

@fhammerl fhammerl left a comment

Choose a reason for hiding this comment

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

LGTM

@nikola-jokic nikola-jokic merged commit c093f87 into main Nov 20, 2023
@nikola-jokic nikola-jokic deleted the nikola-jokic/shell-parse branch November 20, 2023 14:09
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.

3 participants