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 #23 docker-machine restart fails with CentOS VM #24

Merged
merged 1 commit into from
Oct 20, 2016
Merged

Fix #23 docker-machine restart fails with CentOS VM #24

merged 1 commit into from
Oct 20, 2016

Conversation

praveenkumar
Copy link
Collaborator

Restart will now works for machine.

@hferentschik
Copy link
Member

Checking it out ...

@hferentschik hferentschik changed the title Fix #23 Restart shouldn't fail with machine Fix #23 docker-machine restart fails with CentOS VM Oct 18, 2016
@hferentschik hferentschik added this to the POC milestone Oct 18, 2016
Copy link
Member

@hferentschik hferentschik left a comment

Choose a reason for hiding this comment

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

Works like a charm and also addresses #20 :-) Let's get this in asap. I'd like to see some more commentary though around why we do it. Either in the script itself or we start some sort of technical discussions section in the readme where we expand on this and potentially other things we did/do.

@@ -99,6 +100,41 @@ yum remove -y redhat-logos linux-firmware
# Clear yum package and metadata cache
yum clean all

# Place holder for base64 encrypt handle-user-data script
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some comments here to why we do this and how default boot2docker works in comparison which makes it necessary that we do this. Also mentioning that in the case of a 'docker-machine create' this part is basically executed twice. Once as part of the init script here and once by the Red Hat provisioner.

${cert_gen}
EOF

base64 -d < cert-gen.sh.base64 > cert-gen.sh
Copy link
Member

Choose a reason for hiding this comment

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

Technically I think line 104 to 108 are really repetitive and can be automated. One would just write '+${cert_gen}' and one inserts the full monty - cat command, base 63 decode and all.

It would be something like:

${cert_gen}
chmod +x cert-gen.sh
mv cert-gen.sh /opt

where the variable would expand to something like

cat > cert-gen.sh.base64 << EOF
<base 64 encoded script>
EOF
base64 -d < cert-gen.sh.base64 > cert-gen.sh

I think this would be even nicer than what we have. Maybe another issue!?

Copy link
Member

Choose a reason for hiding this comment

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

#25

@@ -0,0 +1,82 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

Can we align the naming of scripts? Either with or without .sh at the end. Personally I can go with both with a slight preference for with .sh. Either way it would be nice to be consistent.

@hferentschik hferentschik removed this from the POC milestone Oct 18, 2016
@hferentschik hferentschik merged commit f1b943e into minishift:master Oct 20, 2016
@hferentschik
Copy link
Member

Fix for issue #23.

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