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

Add apply_cmd_bat to Tiltfile for Windows #155

Closed
wants to merge 2 commits into from

Conversation

andrew-woosnam
Copy link
Contributor

@andrew-woosnam andrew-woosnam commented Dec 14, 2022

fixes #149 (thanks @vrabbi for the solution!)

Prevents errors due to default > /dev/null on windows by replacing it with windows-compatible > NUL

From tilt docs:

command_bat ( Union [ str , List [ str ]]) – If non-empty and on Windows, takes precedence over command . Ignored on other platforms. If a string, executed as a Windows batch command executed with cmd /S /C ; if a list, will be passed to the operating system as program name and args.

- prevents errors due to default `> /dev/null` on windows by replacing it with windows-compatible `> NUL`
- From tilt docs: command_bat ( Union [ str , List [ str ]]) – If non-empty and on Windows, takes precedence over command . Ignored on other platforms. If a string, executed as a Windows batch command executed with cmd /S /C ; if a list, will be passed to the operating system as program name and args.
@andrew-woosnam
Copy link
Contributor Author

Here's a quick demo to show error resolution:

apply_cmd_bat_demo.mp4

@ccheetham
Copy link
Contributor

alternatively to removing output command env var, create 2: one each for cmd and default. Either way, use (or non use) should be consistent

@andrew-woosnam
Copy link
Contributor Author

alternatively to removing output command env var, create 2: one each for cmd and default. Either way, use (or non use) should be consistent

Addressed in b0ed75f -- how does that look @ccheetham?

Copy link
Contributor

@ccheetham ccheetham left a comment

Choose a reason for hiding this comment

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

lgtm

" --build-env BP_DEBUG_ENABLED=true" +
" --namespace " + NAMESPACE +
" --yes " +
" > NUL " + # windows-compatible "output to null"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
" > NUL " + # windows-compatible "output to null"
" > NUL " +

Copy link
Contributor

Choose a reason for hiding this comment

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

Also remove OUTPUT_TO_NULL_COMMAND declaration at line 4

@@ -14,6 +14,14 @@ k8s_custom_deploy(
" --yes " +
OUTPUT_TO_NULL_COMMAND +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OUTPUT_TO_NULL_COMMAND +
" > /dev/null " +

@TimHess
Copy link
Member

TimHess commented Feb 13, 2023

Now that -oyaml has been added to the apps plugin and is used in #188, this PR can probably just be closed

@trisberg
Copy link
Member

closing based on latest comment

@trisberg trisberg closed this Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tiltfile OUTPUT_TO_NULL_COMMAND not recognized on Windows
5 participants