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

Fix issue terraform scripts not working on windows #238

Merged
merged 2 commits into from
Sep 12, 2023

Conversation

dmm9
Copy link
Contributor

@dmm9 dmm9 commented Sep 7, 2023

Theia-cloud does not work on Windows out-of-the-box. Probably due to the fact that the local installation of terraform uses cmd.exe under the hoods. There are 2 issues:

  • --type='json' --> Single or double quotes cause an error. Removing the quotes works on windows.
  • -p '[{ \"... --> Also the initial quotes and escaped quotes cause issues. I tried several combinations with no success. The only way I have found is to extract the patch to a local variable.

I didn't test in linux, I hope the CI tests it. Otherwise please test with linux before merging.

@jfaltermeier jfaltermeier self-requested a review September 8, 2023 09:03
Copy link
Contributor

@jfaltermeier jfaltermeier left a comment

Choose a reason for hiding this comment

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

Hi, thank you very much for the contribution. Unfortunately the sh on linux behaves different than cmd on windows, so the changes don't work on my system.

I've made a suggestion where we check whether the file path starts with /. If so we add the single quotes, otherwise we don't use quotes. I would guess that a windows path will start with something like C:. Could you please test if this works for you as well?

terraform/modules/helm/main.tf Show resolved Hide resolved
terraform/modules/helm/main.tf Outdated Show resolved Hide resolved
Co-authored-by: dmm9 <dmm9@users.noreply.github.com>
@jfaltermeier jfaltermeier merged commit 685e1ec into eclipse-theia:main Sep 12, 2023
7 checks passed
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.

2 participants