Skip to content

Raise an error to the user if the exec command parsed is blank#1405

Merged
djmb merged 3 commits intobasecamp:mainfrom
mike-weiner:fix-1399-fail-exec-without-cmd
Apr 21, 2025
Merged

Raise an error to the user if the exec command parsed is blank#1405
djmb merged 3 commits intobasecamp:mainfrom
mike-weiner:fix-1399-fail-exec-without-cmd

Conversation

@mike-weiner
Copy link
Contributor

@mike-weiner mike-weiner commented Feb 8, 2025 •
edited
Loading

Related to: #1399

weiner % kamal app exec --primary --env=TEST:1 '/usr/bin/echo :test'      
Get most recent version available as an image...
Launching command with version latest from new container...
  INFO [ab8c5ab1] Running docker run --rm --network kamal --env PORT="3000" --env TEST="1" --env /usr/bin/echo ="test" --log-opt max-size="10m" registry.hub.docker.com/<REDACTED>/<REDACTED>:latest  on <REDACTED>
  ERROR (SSHKit::Command::Failed): Exception while executing on host <REDACTED>: docker exit status: 125
docker stdout: Nothing written
docker stderr: docker: invalid reference format.
See 'docker run --help'.

option :env, aliases: "-e", type: :hash, desc: "Set environment variables for the command"

The problem described in #1399 appears to be a side effect of the env option being of type hash.

If the user's command to execute contains : and it appears after the first --env option in the command string, thor appears to try and (incorrectly) treat it as a key/value pair.

There should be a more informative error message returned to the user in this scenario. This problem is avoided altogether if the command to execute inside the container(s) is provided first followed by any options.

@mike-weiner mike-weiner changed the title Fail kamal app exec without a CMD Raise an error to the user if the command parsed is blank Feb 8, 2025
@mike-weiner mike-weiner mentioned this pull request Feb 8, 2025
Open
@mike-weiner mike-weiner changed the title Raise an error to the user if the command parsed is blank Raise an error to the user if the exec command parsed is blank Feb 15, 2025
@mike-weiner
Copy link
Contributor Author

mike-weiner commented Mar 10, 2025

@djmb - This is ready for another workflow approval for PR checks. Thanks!

@djmb djmb merged commit d0c9af2 into basecamp:main Apr 21, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mike-weiner @djmb