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

Addworker cmd #6

Merged
merged 9 commits into from
Feb 13, 2018
Merged

Addworker cmd #6

merged 9 commits into from
Feb 13, 2018

Conversation

cice
Copy link
Contributor

@cice cice commented Feb 5, 2018

Hey, so this is more proof of concept and pretty much my first Go code at all.
So, room for improvement for sure, but maybe we can make sth of it.
Cheers!
Marian

@xetys
Copy link
Owner

xetys commented Feb 6, 2018

Hey! Thanks a lot for your contribution. The code looks very good.

I tested the code and noticed that your version doesn't run the node provisioning in parallel. When you create a new cluster, it directly starts to provision the last worker node. Consequently, it fails to provision an initial cluster.

And a little convention stuff:

  • could you please use 'AddWorker" instead of "Addworker"
  • would you kindly add a cobra description? You can remove the long description 😄

As soon the issues are solved, I'll merge this.

@cice
Copy link
Contributor Author

cice commented Feb 6, 2018

Hey, thanks alot for the feedback. I implemented the requested changes, i think it works properly now.

@xetys
Copy link
Owner

xetys commented Feb 13, 2018

there are some little issues, but I will fix them on my own. LGTM and thanks for your contribution!

@xetys xetys merged commit c00bead into xetys:master Feb 13, 2018
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