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

change the current dir to the config dir #7209

Merged
merged 1 commit into from May 1, 2015
Merged

Conversation

you-n-g
Copy link
Contributor

@you-n-g you-n-g commented Apr 23, 2015

Otherwise the script can't run in other dirs.

@davidopp
Copy link
Member

@erictune assigning to you, please reassign as you wish

@erictune
Copy link
Member

@resouer @WIZARD-CXY can one of you review this, and I'll merge if either of you say it looks good.

@WIZARD-CXY
Copy link
Contributor

Thanks for updating the script @you-n-g , but we have a new approach of deploying k8s on ubuntu as described here. #5498 will be more easy and automatic for user to use.So the file changed will be refactored to another new files. @erictune @resouer. What do you think? Should we merge it ?

@WIZARD-CXY
Copy link
Contributor

I think we don't need it in our approach @resouer.Because the directory will be totally different.

@resouer
Copy link
Contributor

resouer commented Apr 29, 2015

@WIZARD-CXY Always make sure current script is usable until #5498 is merged, although this file will be deprecated in the future, we still need consider this change this time.

@WIZARD-CXY
Copy link
Contributor

Ok, I'm just saying it won't be needed in the new approach and I am busy working on updating #5498 you reviewed it and say LGTM, I'll be ok with it

@resouer
Copy link
Contributor

resouer commented Apr 29, 2015

@you-n-g I added comment, please consider it.

@@ -21,6 +21,10 @@

set -e

CONFIG_DIR=`dirname "$0"`
CONFIG_DIR=`cd "$CONFIG_DIR"; pwd`
Copy link
Contributor

Choose a reason for hiding this comment

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

Does line 25 really needed? Although dirname returns ./ubuntu-cluster, but cd CONFIG_DIR can also works as you expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line 25 will change the path to a full path.
Indeed it makes no difference here between full path and relative path so far. You can disgard that line when merging.
I just habitually do that in my own scripts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Em ... I'm not against that, just keep it. Would u please add comments to your change and then squash to one commit? And then we can ship it.

Otherwise the script can't run in other dirs.
@you-n-g
Copy link
Contributor Author

you-n-g commented Apr 30, 2015

@resouer I added comment by amending and pushed just now. :)

@resouer
Copy link
Contributor

resouer commented Apr 30, 2015

@erictune LGTM

plz ship it whenever you have time, thanks

erictune added a commit that referenced this pull request May 1, 2015
change the current dir to the config dir
@erictune erictune merged commit dc137a4 into kubernetes:master May 1, 2015
@erictune
Copy link
Member

erictune commented May 1, 2015

thanks @resouer for reviwing.
thanks @you-n-g for contrib.

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

6 participants