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

Migrate to terraform 0.12.7 #13

Merged
merged 5 commits into from
Aug 28, 2019
Merged

Conversation

hikhvar
Copy link
Contributor

@hikhvar hikhvar commented Aug 27, 2019

No description provided.

@hikhvar hikhvar changed the title Migrate to terraform 0.12.7 WIP: Migrate to terraform 0.12.7 Aug 27, 2019
@hikhvar hikhvar changed the title WIP: Migrate to terraform 0.12.7 Migrate to terraform 0.12.7 Aug 27, 2019
Copy link
Contributor

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

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

LGTM, just one question about the Docker image.

Dockerfile Outdated Show resolved Hide resolved
Readme.md Outdated
The provided dockerfile set up a system with all required software. To deploy the training environment run:
```bash
docker build -t lfs458-prep
docker run -it -v $(pwd):/wd -w /wd lfs458-prep
Copy link
Contributor

Choose a reason for hiding this comment

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

From my pov this is more work than just downloading the according binary and use it. If we use the official docker image IMHO this can work since we don't need anything else (except docker of course).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I would add --rm since we only need an "ephemeral" container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for most people this is more work. Me included. However, I wanted to provide a stable environment for the terraform code and the create_package.sh script. Therefore the dockerfile include puttygen and zip.

Readme.md Outdated Show resolved Hide resolved
@hikhvar
Copy link
Contributor Author

hikhvar commented Aug 28, 2019

PTAL

Copy link
Contributor

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

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

Just some Markdown issues and a simplified apk comman

Dockerfile Outdated Show resolved Hide resolved
Readme.md Show resolved Hide resolved
Readme.md Show resolved Hide resolved
Readme.md Outdated Show resolved Hide resolved
Readme.md Show resolved Hide resolved
Copy link
Contributor

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@johscheuer johscheuer merged commit d6dbdc9 into inovex:master Aug 28, 2019
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.

None yet

2 participants