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

Conversation

Projects
None yet
6 participants
@you-n-g
Copy link
Contributor

you-n-g commented Apr 23, 2015

Otherwise the script can't run in other dirs.

@googlebot googlebot added the cla: yes label Apr 23, 2015

@davidopp

This comment has been minimized.

Copy link
Member

davidopp commented Apr 24, 2015

@erictune assigning to you, please reassign as you wish

@erictune

This comment has been minimized.

Copy link
Member

erictune commented Apr 29, 2015

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

@WIZARD-CXY

This comment has been minimized.

Copy link
Contributor

WIZARD-CXY commented Apr 29, 2015

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

This comment has been minimized.

Copy link
Contributor

WIZARD-CXY commented Apr 29, 2015

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

@resouer

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

WIZARD-CXY commented Apr 29, 2015

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

This comment has been minimized.

Copy link
Member

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`

This comment has been minimized.

Copy link
@resouer

resouer Apr 29, 2015

Member

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

This comment has been minimized.

Copy link
@you-n-g

you-n-g Apr 29, 2015

Author Contributor

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.

This comment has been minimized.

Copy link
@resouer

resouer Apr 30, 2015

Member

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.

change the current dir to the config dir
Otherwise the script can't run in other dirs.

@you-n-g you-n-g force-pushed the you-n-g:master branch from 0976bf9 to f181e98 Apr 30, 2015

@you-n-g

This comment has been minimized.

Copy link
Contributor Author

you-n-g commented Apr 30, 2015

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

@resouer

This comment has been minimized.

Copy link
Member

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

Merge pull request #7209 from you-n-g/master
change the current dir to the config dir

@erictune erictune merged commit dc137a4 into kubernetes:master May 1, 2015

2 of 4 checks passed

Shippable Builds timed-out on Shippable
Details
coverage/coveralls Coverage decreased (-0.01%) to 48.46%
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@erictune

This comment has been minimized.

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
You can’t perform that action at this time.