Fixes Setup.py error on Windows #676 - #734

Closed
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom
Sai-Adarsh:master
Closed

Fixes Setup.py error on Windows #676#734
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom
Sai-Adarsh:master

Conversation

@Sai-Adarsh

@Sai-AdarshSai-Adarsh commented Jan 21, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes Setup.py error on Windows, added shell script to check python environment, prints the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install issue #676

@Sai-Adarsh
Fixes Setup,py error on Windows#676
4a3c232
Fixes Setup,py error on Windows, added shell script to check python environment, print the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install
@k8s-ci-robotk8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 21, 2019
@k8s-ci-robot

k8s-ci-robot commented Jan 21, 2019

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Sai-Adarsh
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: roycaihw

If they are not already assigned, you can assign the PR to them by writing /assign @roycaihw in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robotk8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 21, 2019
@Sai-Adarsh

Sai-Adarsh commented Jan 21, 2019

Copy link
Copy Markdown
ContributorAuthor

@micw523 gave a PR. could you check this out.

@Sai-Adarsh

Sai-Adarsh commented Jan 21, 2019

Copy link
Copy Markdown
ContributorAuthor

/assign @roycaihw

@Sai-Adarsh

Sai-Adarsh commented Jan 21, 2019

Copy link
Copy Markdown
ContributorAuthor

/assign @micw523

@Sai-AdarshSai-Adarsh changed the title Fixes Setup,py error on Windows #676Fixes Setup.py error on Windows #676Jan 21, 2019
@k8s-ci-robot

k8s-ci-robot commented Jan 21, 2019

Copy link
Copy Markdown
Contributor

@Sai-Adarsh: you cannot LGTM your own PR.

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot

k8s-ci-robot commented Jan 21, 2019

Copy link
Copy Markdown
Contributor

@Appendly: changing LGTM is restricted to assignees, and only kubernetes-client/python repo collaborators may be assigned issues.

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

yliaog
yliaog reviewed Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat Outdated
echo Python environment found..
fi
python --version
cp -R ../kubernetes/base/* ../kubernetes/

@yliaogyliaogJan 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?

@Sai-AdarshSai-AdarshJan 22, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?

"When we clone submodule with git the folders in the /kubernetes/base is supposed to be symbolically linked in the /kubernetes directory. In Windows this is not done. For a Windows developer, we’ll need to have a script copying all the base/* folders into */. "

^ is what @micw523 requested in his issue. so 'cp -R' does the job.

Comment threadscripts/setup-fix.bat Outdated
@@ -0,0 +1,11 @@
if ! [ -x "$(command -v python)" ];
then
echo Python environment notfound..

@yliaogyliaogJan 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

probably add double quotes around the echo string

Comment threadscripts/setup-fix.bat Outdated
then
echo Python environment notfound..
else
echo Python environment found..

@yliaogyliaogJan 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ditto

@Sai-AdarshSai-AdarshJan 22, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ditto

cool. will do the necessary changes

@micw523micw523Jan 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo Python environment found..
echo Python environment found.

micw523
micw523 suggested changes Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat Outdated
Comment threadscripts/setup-fix.bat Outdated
echo "Python environment notfound.."
else
echo "Python environment found.."
fi

@micw523micw523Jan 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please check windows batch scripting syntax for if

@micw523

micw523 commented Jan 22, 2019

Copy link
Copy Markdown
Contributor

To be honest this looks like bash instead of a Windows bat. You could probably use it in a Cygwin but I think Windows developers would prefer a genuine bat.

@k8s-ci-robotk8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 24, 2019
@Sai-Adarsh

Sai-Adarsh commented Jan 24, 2019

Copy link
Copy Markdown
ContributorAuthor

@micw523 done with the changes. could you check on the new commit :)

micw523
micw523 reviewed Jan 24, 2019
View reviewed changes

@micw523micw523 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have a few comments -

  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?

@Sai-Adarsh

Sai-Adarsh commented Jan 24, 2019

Copy link
Copy Markdown
ContributorAuthor

I have a few comments -

  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?

done with the changes.

micw523
micw523 reviewed Jan 24, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat Outdated
@@ -0,0 +1,5 @@
( where python >null
echo Python environment found..

@micw523micw523Jan 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In this case Python environment found will be printed regardless.

@micw523micw523Jan 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

python > null and found.

@Sai-AdarshSai-AdarshJan 24, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

python > null and found.

cool. done with the changes.

micw523
micw523 reviewed Jan 24, 2019
View reviewed changes

@micw523micw523 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please fix your indents - the conditional statements are not aligned.
Also, one period for output message is sufficient.
On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough

@Sai-Adarsh

Sai-Adarsh commented Jan 24, 2019

Copy link
Copy Markdown
ContributorAuthor

Please fix your indents - the conditional statements are not aligned.
Also, one period for output message is sufficient.
On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough

done.

micw523
micw523 reviewed Jan 28, 2019
View reviewed changes

@micw523micw523 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See suggestion

@Sai-Adarsh

Sai-Adarsh commented Jan 28, 2019

Copy link
Copy Markdown
ContributorAuthor

See suggestion

done with changes.

@micw523

micw523 commented Jan 28, 2019

Copy link
Copy Markdown
Contributor

/lgtm
/cc @roycaihw

@k8s-ci-robotk8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2019
roycaihw
roycaihw reviewed Jan 29, 2019
View reviewed changes

@roycaihwroycaihw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/hold

Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise

Comment threadscripts/setup-fix.bat Outdated
@@ -0,0 +1,6 @@
( python --version>nul 2>&1 && (
echo Python environment found.

@roycaihwroycaihwJan 29, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?

@Sai-AdarshSai-AdarshJan 29, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?

works fine without double qoute, so thought of reverting the change :)

@k8s-ci-robotk8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2019
@Sai-Adarsh

Sai-Adarsh commented Jan 29, 2019

Copy link
Copy Markdown
ContributorAuthor

/hold

Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise

cool.

@k8s-ci-robot

k8s-ci-robot commented Jan 29, 2019

Copy link
Copy Markdown
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robotk8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2019
@Sai-Adarsh

Sai-Adarsh commented Jan 29, 2019

Copy link
Copy Markdown
ContributorAuthor

/hold

Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise

@roycaihw could you check on the new changes?

@roycaihw

roycaihw commented Jan 29, 2019

Copy link
Copy Markdown
Member

Could you add the comments to the beginning of the file? Basically I was asking for a description so that people can tell what it's for and when it should be used. I think we don't need inline comments.

Something like "Install python client in Windows. Windows doesn't have symbolic link that this repo uses. This batch script provides a workaround for symbolic link by copying the folders over."

Also please squash your commits before submit

/hold cancel

@k8s-ci-robotk8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2019
@k8s-ci-robotk8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 30, 2019
@micw523

micw523 commented Feb 7, 2019

Copy link
Copy Markdown
Contributor

Will lg()tm after squash

@Sai-Adarsh

Sai-Adarsh commented Feb 13, 2019

Copy link
Copy Markdown
ContributorAuthor

Will lg()tm after squash

cool.

@Sai-AdarshSai-Adarsh mentioned this pull request Feb 20, 2019
Merged
@Sai-AdarshSai-Adarsh mentioned this pull request Mar 8, 2019
Closed
Sai-Adarsh added a commit to Sai-Adarsh/python that referenced this pull request Mar 9, 2019
@Sai-Adarsh
Updating kubernetes-client owners with sai-adarsh
e18794c
Would like to join a GitHub team of kubernetes-client, and highly interested to join as a reviewer. Have previously merged a fix (kubernetes-client#734).
@Sai-AdarshSai-Adarsh mentioned this pull request Mar 9, 2019
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roycaihwroycaihwroycaihw left review comments
@yliaogyliaogyliaog left review comments
@mbohloolmbohloolAwaiting requested review from mbohlool
+1 more reviewer
@micw523micw523micw523 requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@roycaihwroycaihw

@micw523micw523

Labels

cncf-cla: yesIndicates the PR's author has signed the CNCF CLA.size/SDenotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Sai-Adarsh@k8s-ci-robot@micw523@roycaihw@yliaog
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Fixes Setup.py error on Windows #676 - #734
Closed
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Closed
Fixes Setup.py error on Windows #676#734
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Conversation
@Sai-Adarsh
@Sai-AdarshSai-Adarsh
commented
Jan 21, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes Setup.py error on Windows, added shell script to check python environment, prints the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install issue #676
@Sai-Adarsh
Fixes Setup,py error on Windows#676
4a3c232
Fixes Setup,py error on Windows, added shell script to check python environment, print the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install
@k8s-ci-robotk8s-ci-robot
added
the 
size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
label
Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Sai-Adarsh

To fully approve this pull request, please assign additional approvers.

We suggest the following additional approver: roycaihw
If they are not already assigned, you can assign the PR to them by writing /assign @roycaihw in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment

Approvers can cancel approval by writing /approve cancel in a comment
@k8s-ci-robotk8s-ci-robot
added
the 
cncf-cla: yes
Indicates the PR's author has signed the CNCF CLA.
label
Jan 21, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 gave a PR. could you check this out.
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @roycaihw
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @micw523
@Sai-AdarshSai-Adarsh
changed the title
Fixes Setup,py error on Windows #676Fixes Setup.py error on Windows #676Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Sai-Adarsh: you cannot LGTM your own PR.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Appendly: changing LGTM is restricted to assignees, and only kubernetes-client/python repo collaborators may be assigned issues.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
yliaog
yliaog
reviewed
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

	echo Python environment found..
fi
python --version
cp -R ../kubernetes/base/* ../kubernetes/
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
"When we clone submodule with git the folders in the /kubernetes/base is supposed to be symbolically linked in the /kubernetes directory. In Windows this is not done. For a Windows developer, we’ll need to have a script copying all the base/* folders into */. "
^ is what @micw523 requested in his issue. so 'cp -R' does the job.
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,11 @@
if ! [ -x "$(command -v python)" ];
then 
	echo Python environment notfound..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

probably add double quotes around the echo string
Comment threadscripts/setup-fix.bat
Outdated

then 
	echo Python environment notfound..
else 
	echo Python environment found..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ditto
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ditto
cool. will do the necessary changes
@micw523micw523Jan 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

echo Python environment found..
echo Python environment found.
micw523
micw523
suggested changes
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

Comment threadscripts/setup-fix.bat
Outdated

	echo "Python environment notfound.."
else 
	echo "Python environment found.."
fi
@micw523micw523Jan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please check windows batch scripting syntax for if
@micw523
micw523
commented
Jan 22, 2019

Copy link


Copy Markdown

Contributor
To be honest this looks like bash instead of a Windows bat. You could probably use it in a Cygwin but I think Windows developers would prefer a genuine bat.
@k8s-ci-robotk8s-ci-robot
added

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
and removed

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
labels
Jan 24, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 done with the changes. could you check on the new commit :)
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,5 @@
( where python >null
	echo Python environment found..
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In this case Python environment found will be printed regardless.
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

python > null and found.
@Sai-AdarshSai-AdarshJan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

python > null and found.
cool. done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
done.
micw523
micw523
reviewed
Jan 28, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

See suggestion
@Sai-Adarsh
Sai-Adarsh
commented
Jan 28, 2019

Copy link


Copy Markdown

ContributorAuthor
See suggestion
done with changes.
@micw523
micw523
commented
Jan 28, 2019

Copy link


Copy Markdown

Contributor
/lgtm

/cc @roycaihw
@k8s-ci-robotk8s-ci-robot
added
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 28, 2019
roycaihw
roycaihw
reviewed
Jan 29, 2019
View reviewed changes
@roycaihwroycaihw
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,6 @@
(	python --version>nul 2>&1 && (
	echo Python environment found.
@roycaihwroycaihwJan 29, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
@Sai-AdarshSai-AdarshJan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
works fine without double qoute, so thought of reverting the change :)
@k8s-ci-robotk8s-ci-robot
added
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
cool.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 29, 2019

Copy link


Copy Markdown

Contributor
New changes are detected. LGTM label has been removed.
@k8s-ci-robotk8s-ci-robot
removed
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
@roycaihw could you check on the new changes?
@roycaihw
roycaihw
commented
Jan 29, 2019

Copy link


Copy Markdown

Member
Could you add the comments to the beginning of the file? Basically I was asking for a description so that people can tell what it's for and when it should be used. I think we don't need inline comments.
Something like "Install python client in Windows. Windows doesn't have symbolic link that this repo uses. This batch script provides a workaround for symbolic link by copying the folders over."
Also please squash your commits before submit
/hold cancel
@k8s-ci-robotk8s-ci-robot
removed
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@k8s-ci-robotk8s-ci-robot
added

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
and removed

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
labels
Jan 30, 2019
@micw523
micw523
commented
Feb 7, 2019

Copy link


Copy Markdown

Contributor
Will lg()tm after squash
@Sai-Adarsh
Sai-Adarsh
commented
Feb 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Will lg()tm after squash
cool.
@Sai-AdarshSai-Adarsh
mentioned this pull request
Feb 20, 2019
 Merged

@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 8, 2019
 Closed

Sai-Adarsh added a commit
to Sai-Adarsh/python
that referenced
this pull request
Mar 9, 2019
@Sai-Adarsh
Updating kubernetes-client owners with sai-adarsh
e18794c
Would like to join a GitHub team of kubernetes-client, and highly interested to join as a reviewer. Have previously merged a fix (kubernetes-client#734).
@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 9, 2019
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@roycaihwroycaihwroycaihw left review comments
@yliaogyliaogyliaog left review comments
@mbohloolmbohloolAwaiting requested review from mbohlool

+1 more reviewer

@micw523micw523micw523 requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@roycaihwroycaihw
@micw523micw523

Labels

cncf-cla: yesIndicates the PR's author has signed the CNCF CLA.size/SDenotes a PR that changes 10-29 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@Sai-Adarsh@k8s-ci-robot@micw523@roycaihw@yliaog

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fixes Setup.py error on Windows #676 - #734
Closed
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Closed
Fixes Setup.py error on Windows #676#734
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Conversation
@Sai-Adarsh
@Sai-AdarshSai-Adarsh
commented
Jan 21, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes Setup.py error on Windows, added shell script to check python environment, prints the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install issue #676
@Sai-Adarsh
Fixes Setup,py error on Windows#676
4a3c232
Fixes Setup,py error on Windows, added shell script to check python environment, print the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install
@k8s-ci-robotk8s-ci-robot
added
the 
size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
label
Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Sai-Adarsh

To fully approve this pull request, please assign additional approvers.

We suggest the following additional approver: roycaihw
If they are not already assigned, you can assign the PR to them by writing /assign @roycaihw in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment

Approvers can cancel approval by writing /approve cancel in a comment
@k8s-ci-robotk8s-ci-robot
added
the 
cncf-cla: yes
Indicates the PR's author has signed the CNCF CLA.
label
Jan 21, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 gave a PR. could you check this out.
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @roycaihw
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @micw523
@Sai-AdarshSai-Adarsh
changed the title
Fixes Setup,py error on Windows #676Fixes Setup.py error on Windows #676Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Sai-Adarsh: you cannot LGTM your own PR.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Appendly: changing LGTM is restricted to assignees, and only kubernetes-client/python repo collaborators may be assigned issues.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
yliaog
yliaog
reviewed
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

	echo Python environment found..
fi
python --version
cp -R ../kubernetes/base/* ../kubernetes/
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
"When we clone submodule with git the folders in the /kubernetes/base is supposed to be symbolically linked in the /kubernetes directory. In Windows this is not done. For a Windows developer, we’ll need to have a script copying all the base/* folders into */. "
^ is what @micw523 requested in his issue. so 'cp -R' does the job.
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,11 @@
if ! [ -x "$(command -v python)" ];
then 
	echo Python environment notfound..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

probably add double quotes around the echo string
Comment threadscripts/setup-fix.bat
Outdated

then 
	echo Python environment notfound..
else 
	echo Python environment found..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ditto
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ditto
cool. will do the necessary changes
@micw523micw523Jan 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

echo Python environment found..
echo Python environment found.
micw523
micw523
suggested changes
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

Comment threadscripts/setup-fix.bat
Outdated

	echo "Python environment notfound.."
else 
	echo "Python environment found.."
fi
@micw523micw523Jan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please check windows batch scripting syntax for if
@micw523
micw523
commented
Jan 22, 2019

Copy link


Copy Markdown

Contributor
To be honest this looks like bash instead of a Windows bat. You could probably use it in a Cygwin but I think Windows developers would prefer a genuine bat.
@k8s-ci-robotk8s-ci-robot
added

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
and removed

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
labels
Jan 24, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 done with the changes. could you check on the new commit :)
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,5 @@
( where python >null
	echo Python environment found..
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In this case Python environment found will be printed regardless.
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

python > null and found.
@Sai-AdarshSai-AdarshJan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

python > null and found.
cool. done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
done.
micw523
micw523
reviewed
Jan 28, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

See suggestion
@Sai-Adarsh
Sai-Adarsh
commented
Jan 28, 2019

Copy link


Copy Markdown

ContributorAuthor
See suggestion
done with changes.
@micw523
micw523
commented
Jan 28, 2019

Copy link


Copy Markdown

Contributor
/lgtm

/cc @roycaihw
@k8s-ci-robotk8s-ci-robot
added
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 28, 2019
roycaihw
roycaihw
reviewed
Jan 29, 2019
View reviewed changes
@roycaihwroycaihw
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,6 @@
(	python --version>nul 2>&1 && (
	echo Python environment found.
@roycaihwroycaihwJan 29, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
@Sai-AdarshSai-AdarshJan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
works fine without double qoute, so thought of reverting the change :)
@k8s-ci-robotk8s-ci-robot
added
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
cool.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 29, 2019

Copy link


Copy Markdown

Contributor
New changes are detected. LGTM label has been removed.
@k8s-ci-robotk8s-ci-robot
removed
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
@roycaihw could you check on the new changes?
@roycaihw
roycaihw
commented
Jan 29, 2019

Copy link


Copy Markdown

Member
Could you add the comments to the beginning of the file? Basically I was asking for a description so that people can tell what it's for and when it should be used. I think we don't need inline comments.
Something like "Install python client in Windows. Windows doesn't have symbolic link that this repo uses. This batch script provides a workaround for symbolic link by copying the folders over."
Also please squash your commits before submit
/hold cancel
@k8s-ci-robotk8s-ci-robot
removed
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@k8s-ci-robotk8s-ci-robot
added

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
and removed

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
labels
Jan 30, 2019
@micw523
micw523
commented
Feb 7, 2019

Copy link


Copy Markdown

Contributor
Will lg()tm after squash
@Sai-Adarsh
Sai-Adarsh
commented
Feb 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Will lg()tm after squash
cool.
@Sai-AdarshSai-Adarsh
mentioned this pull request
Feb 20, 2019
 Merged

@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 8, 2019
 Closed

Sai-Adarsh added a commit
to Sai-Adarsh/python
that referenced
this pull request
Mar 9, 2019
@Sai-Adarsh
Updating kubernetes-client owners with sai-adarsh
e18794c
Would like to join a GitHub team of kubernetes-client, and highly interested to join as a reviewer. Have previously merged a fix (kubernetes-client#734).
@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 9, 2019
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@roycaihwroycaihwroycaihw left review comments
@yliaogyliaogyliaog left review comments
@mbohloolmbohloolAwaiting requested review from mbohlool

+1 more reviewer

@micw523micw523micw523 requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@roycaihwroycaihw
@micw523micw523

Labels

cncf-cla: yesIndicates the PR's author has signed the CNCF CLA.size/SDenotes a PR that changes 10-29 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@Sai-Adarsh@k8s-ci-robot@micw523@roycaihw@yliaog

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fixes Setup.py error on Windows #676 - #734
Closed
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Closed
Fixes Setup.py error on Windows #676#734
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Conversation
@Sai-Adarsh
@Sai-AdarshSai-Adarsh
commented
Jan 21, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes Setup.py error on Windows, added shell script to check python environment, prints the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install issue #676
@Sai-Adarsh
Fixes Setup,py error on Windows#676
4a3c232
Fixes Setup,py error on Windows, added shell script to check python environment, print the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install
@k8s-ci-robotk8s-ci-robot
added
the 
size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
label
Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Sai-Adarsh

To fully approve this pull request, please assign additional approvers.

We suggest the following additional approver: roycaihw
If they are not already assigned, you can assign the PR to them by writing /assign @roycaihw in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment

Approvers can cancel approval by writing /approve cancel in a comment
@k8s-ci-robotk8s-ci-robot
added
the 
cncf-cla: yes
Indicates the PR's author has signed the CNCF CLA.
label
Jan 21, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 gave a PR. could you check this out.
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @roycaihw
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @micw523
@Sai-AdarshSai-Adarsh
changed the title
Fixes Setup,py error on Windows #676Fixes Setup.py error on Windows #676Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Sai-Adarsh: you cannot LGTM your own PR.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Appendly: changing LGTM is restricted to assignees, and only kubernetes-client/python repo collaborators may be assigned issues.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
yliaog
yliaog
reviewed
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

	echo Python environment found..
fi
python --version
cp -R ../kubernetes/base/* ../kubernetes/
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
"When we clone submodule with git the folders in the /kubernetes/base is supposed to be symbolically linked in the /kubernetes directory. In Windows this is not done. For a Windows developer, we’ll need to have a script copying all the base/* folders into */. "
^ is what @micw523 requested in his issue. so 'cp -R' does the job.
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,11 @@
if ! [ -x "$(command -v python)" ];
then 
	echo Python environment notfound..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

probably add double quotes around the echo string
Comment threadscripts/setup-fix.bat
Outdated

then 
	echo Python environment notfound..
else 
	echo Python environment found..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ditto
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ditto
cool. will do the necessary changes
@micw523micw523Jan 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

echo Python environment found..
echo Python environment found.
micw523
micw523
suggested changes
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

Comment threadscripts/setup-fix.bat
Outdated

	echo "Python environment notfound.."
else 
	echo "Python environment found.."
fi
@micw523micw523Jan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please check windows batch scripting syntax for if
@micw523
micw523
commented
Jan 22, 2019

Copy link


Copy Markdown

Contributor
To be honest this looks like bash instead of a Windows bat. You could probably use it in a Cygwin but I think Windows developers would prefer a genuine bat.
@k8s-ci-robotk8s-ci-robot
added

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
and removed

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
labels
Jan 24, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 done with the changes. could you check on the new commit :)
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,5 @@
( where python >null
	echo Python environment found..
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In this case Python environment found will be printed regardless.
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

python > null and found.
@Sai-AdarshSai-AdarshJan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

python > null and found.
cool. done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
done.
micw523
micw523
reviewed
Jan 28, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

See suggestion
@Sai-Adarsh
Sai-Adarsh
commented
Jan 28, 2019

Copy link


Copy Markdown

ContributorAuthor
See suggestion
done with changes.
@micw523
micw523
commented
Jan 28, 2019

Copy link


Copy Markdown

Contributor
/lgtm

/cc @roycaihw
@k8s-ci-robotk8s-ci-robot
added
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 28, 2019
roycaihw
roycaihw
reviewed
Jan 29, 2019
View reviewed changes
@roycaihwroycaihw
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,6 @@
(	python --version>nul 2>&1 && (
	echo Python environment found.
@roycaihwroycaihwJan 29, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
@Sai-AdarshSai-AdarshJan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
works fine without double qoute, so thought of reverting the change :)
@k8s-ci-robotk8s-ci-robot
added
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
cool.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 29, 2019

Copy link


Copy Markdown

Contributor
New changes are detected. LGTM label has been removed.
@k8s-ci-robotk8s-ci-robot
removed
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
@roycaihw could you check on the new changes?
@roycaihw
roycaihw
commented
Jan 29, 2019

Copy link


Copy Markdown

Member
Could you add the comments to the beginning of the file? Basically I was asking for a description so that people can tell what it's for and when it should be used. I think we don't need inline comments.
Something like "Install python client in Windows. Windows doesn't have symbolic link that this repo uses. This batch script provides a workaround for symbolic link by copying the folders over."
Also please squash your commits before submit
/hold cancel
@k8s-ci-robotk8s-ci-robot
removed
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@k8s-ci-robotk8s-ci-robot
added

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
and removed

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
labels
Jan 30, 2019
@micw523
micw523
commented
Feb 7, 2019

Copy link


Copy Markdown

Contributor
Will lg()tm after squash
@Sai-Adarsh
Sai-Adarsh
commented
Feb 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Will lg()tm after squash
cool.
@Sai-AdarshSai-Adarsh
mentioned this pull request
Feb 20, 2019
 Merged

@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 8, 2019
 Closed

Sai-Adarsh added a commit
to Sai-Adarsh/python
that referenced
this pull request
Mar 9, 2019
@Sai-Adarsh
Updating kubernetes-client owners with sai-adarsh
e18794c
Would like to join a GitHub team of kubernetes-client, and highly interested to join as a reviewer. Have previously merged a fix (kubernetes-client#734).
@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 9, 2019
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@roycaihwroycaihwroycaihw left review comments
@yliaogyliaogyliaog left review comments
@mbohloolmbohloolAwaiting requested review from mbohlool

+1 more reviewer

@micw523micw523micw523 requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@roycaihwroycaihw
@micw523micw523

Labels

cncf-cla: yesIndicates the PR's author has signed the CNCF CLA.size/SDenotes a PR that changes 10-29 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@Sai-Adarsh@k8s-ci-robot@micw523@roycaihw@yliaog

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Fixes Setup.py error on Windows #676 - #734
Closed
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Closed
Fixes Setup.py error on Windows #676#734
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Conversation
@Sai-Adarsh
@Sai-AdarshSai-Adarsh
commented
Jan 21, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes Setup.py error on Windows, added shell script to check python environment, prints the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install issue #676
@Sai-Adarsh
Fixes Setup,py error on Windows#676
4a3c232
Fixes Setup,py error on Windows, added shell script to check python environment, print the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install
@k8s-ci-robotk8s-ci-robot
added
the 
size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
label
Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Sai-Adarsh

To fully approve this pull request, please assign additional approvers.

We suggest the following additional approver: roycaihw
If they are not already assigned, you can assign the PR to them by writing /assign @roycaihw in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment

Approvers can cancel approval by writing /approve cancel in a comment
@k8s-ci-robotk8s-ci-robot
added
the 
cncf-cla: yes
Indicates the PR's author has signed the CNCF CLA.
label
Jan 21, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 gave a PR. could you check this out.
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @roycaihw
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @micw523
@Sai-AdarshSai-Adarsh
changed the title
Fixes Setup,py error on Windows #676Fixes Setup.py error on Windows #676Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Sai-Adarsh: you cannot LGTM your own PR.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Appendly: changing LGTM is restricted to assignees, and only kubernetes-client/python repo collaborators may be assigned issues.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
yliaog
yliaog
reviewed
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

	echo Python environment found..
fi
python --version
cp -R ../kubernetes/base/* ../kubernetes/
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
"When we clone submodule with git the folders in the /kubernetes/base is supposed to be symbolically linked in the /kubernetes directory. In Windows this is not done. For a Windows developer, we’ll need to have a script copying all the base/* folders into */. "
^ is what @micw523 requested in his issue. so 'cp -R' does the job.
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,11 @@
if ! [ -x "$(command -v python)" ];
then 
	echo Python environment notfound..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

probably add double quotes around the echo string
Comment threadscripts/setup-fix.bat
Outdated

then 
	echo Python environment notfound..
else 
	echo Python environment found..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ditto
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ditto
cool. will do the necessary changes
@micw523micw523Jan 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

echo Python environment found..
echo Python environment found.
micw523
micw523
suggested changes
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

Comment threadscripts/setup-fix.bat
Outdated

	echo "Python environment notfound.."
else 
	echo "Python environment found.."
fi
@micw523micw523Jan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please check windows batch scripting syntax for if
@micw523
micw523
commented
Jan 22, 2019

Copy link


Copy Markdown

Contributor
To be honest this looks like bash instead of a Windows bat. You could probably use it in a Cygwin but I think Windows developers would prefer a genuine bat.
@k8s-ci-robotk8s-ci-robot
added

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
and removed

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
labels
Jan 24, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 done with the changes. could you check on the new commit :)
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,5 @@
( where python >null
	echo Python environment found..
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In this case Python environment found will be printed regardless.
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

python > null and found.
@Sai-AdarshSai-AdarshJan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

python > null and found.
cool. done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
done.
micw523
micw523
reviewed
Jan 28, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

See suggestion
@Sai-Adarsh
Sai-Adarsh
commented
Jan 28, 2019

Copy link


Copy Markdown

ContributorAuthor
See suggestion
done with changes.
@micw523
micw523
commented
Jan 28, 2019

Copy link


Copy Markdown

Contributor
/lgtm

/cc @roycaihw
@k8s-ci-robotk8s-ci-robot
added
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 28, 2019
roycaihw
roycaihw
reviewed
Jan 29, 2019
View reviewed changes
@roycaihwroycaihw
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,6 @@
(	python --version>nul 2>&1 && (
	echo Python environment found.
@roycaihwroycaihwJan 29, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
@Sai-AdarshSai-AdarshJan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
works fine without double qoute, so thought of reverting the change :)
@k8s-ci-robotk8s-ci-robot
added
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
cool.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 29, 2019

Copy link


Copy Markdown

Contributor
New changes are detected. LGTM label has been removed.
@k8s-ci-robotk8s-ci-robot
removed
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
@roycaihw could you check on the new changes?
@roycaihw
roycaihw
commented
Jan 29, 2019

Copy link


Copy Markdown

Member
Could you add the comments to the beginning of the file? Basically I was asking for a description so that people can tell what it's for and when it should be used. I think we don't need inline comments.
Something like "Install python client in Windows. Windows doesn't have symbolic link that this repo uses. This batch script provides a workaround for symbolic link by copying the folders over."
Also please squash your commits before submit
/hold cancel
@k8s-ci-robotk8s-ci-robot
removed
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@k8s-ci-robotk8s-ci-robot
added

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
and removed

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
labels
Jan 30, 2019
@micw523
micw523
commented
Feb 7, 2019

Copy link


Copy Markdown

Contributor
Will lg()tm after squash
@Sai-Adarsh
Sai-Adarsh
commented
Feb 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Will lg()tm after squash
cool.
@Sai-AdarshSai-Adarsh
mentioned this pull request
Feb 20, 2019
 Merged

@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 8, 2019
 Closed

Sai-Adarsh added a commit
to Sai-Adarsh/python
that referenced
this pull request
Mar 9, 2019
@Sai-Adarsh
Updating kubernetes-client owners with sai-adarsh
e18794c
Would like to join a GitHub team of kubernetes-client, and highly interested to join as a reviewer. Have previously merged a fix (kubernetes-client#734).
@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 9, 2019
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@roycaihwroycaihwroycaihw left review comments
@yliaogyliaogyliaog left review comments
@mbohloolmbohloolAwaiting requested review from mbohlool

+1 more reviewer

@micw523micw523micw523 requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@roycaihwroycaihw
@micw523micw523

Labels

cncf-cla: yesIndicates the PR's author has signed the CNCF CLA.size/SDenotes a PR that changes 10-29 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@Sai-Adarsh@k8s-ci-robot@micw523@roycaihw@yliaog

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fixes Setup.py error on Windows #676 - #734
Closed
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Closed
Fixes Setup.py error on Windows #676#734
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Conversation
@Sai-Adarsh
@Sai-AdarshSai-Adarsh
commented
Jan 21, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes Setup.py error on Windows, added shell script to check python environment, prints the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install issue #676
@Sai-Adarsh
Fixes Setup,py error on Windows#676
4a3c232
Fixes Setup,py error on Windows, added shell script to check python environment, print the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install
@k8s-ci-robotk8s-ci-robot
added
the 
size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
label
Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Sai-Adarsh

To fully approve this pull request, please assign additional approvers.

We suggest the following additional approver: roycaihw
If they are not already assigned, you can assign the PR to them by writing /assign @roycaihw in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment

Approvers can cancel approval by writing /approve cancel in a comment
@k8s-ci-robotk8s-ci-robot
added
the 
cncf-cla: yes
Indicates the PR's author has signed the CNCF CLA.
label
Jan 21, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 gave a PR. could you check this out.
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @roycaihw
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @micw523
@Sai-AdarshSai-Adarsh
changed the title
Fixes Setup,py error on Windows #676Fixes Setup.py error on Windows #676Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Sai-Adarsh: you cannot LGTM your own PR.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Appendly: changing LGTM is restricted to assignees, and only kubernetes-client/python repo collaborators may be assigned issues.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
yliaog
yliaog
reviewed
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

	echo Python environment found..
fi
python --version
cp -R ../kubernetes/base/* ../kubernetes/
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
"When we clone submodule with git the folders in the /kubernetes/base is supposed to be symbolically linked in the /kubernetes directory. In Windows this is not done. For a Windows developer, we’ll need to have a script copying all the base/* folders into */. "
^ is what @micw523 requested in his issue. so 'cp -R' does the job.
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,11 @@
if ! [ -x "$(command -v python)" ];
then 
	echo Python environment notfound..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

probably add double quotes around the echo string
Comment threadscripts/setup-fix.bat
Outdated

then 
	echo Python environment notfound..
else 
	echo Python environment found..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ditto
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ditto
cool. will do the necessary changes
@micw523micw523Jan 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

echo Python environment found..
echo Python environment found.
micw523
micw523
suggested changes
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

Comment threadscripts/setup-fix.bat
Outdated

	echo "Python environment notfound.."
else 
	echo "Python environment found.."
fi
@micw523micw523Jan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please check windows batch scripting syntax for if
@micw523
micw523
commented
Jan 22, 2019

Copy link


Copy Markdown

Contributor
To be honest this looks like bash instead of a Windows bat. You could probably use it in a Cygwin but I think Windows developers would prefer a genuine bat.
@k8s-ci-robotk8s-ci-robot
added

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
and removed

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
labels
Jan 24, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 done with the changes. could you check on the new commit :)
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,5 @@
( where python >null
	echo Python environment found..
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In this case Python environment found will be printed regardless.
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

python > null and found.
@Sai-AdarshSai-AdarshJan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

python > null and found.
cool. done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
done.
micw523
micw523
reviewed
Jan 28, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

See suggestion
@Sai-Adarsh
Sai-Adarsh
commented
Jan 28, 2019

Copy link


Copy Markdown

ContributorAuthor
See suggestion
done with changes.
@micw523
micw523
commented
Jan 28, 2019

Copy link


Copy Markdown

Contributor
/lgtm

/cc @roycaihw
@k8s-ci-robotk8s-ci-robot
added
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 28, 2019
roycaihw
roycaihw
reviewed
Jan 29, 2019
View reviewed changes
@roycaihwroycaihw
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,6 @@
(	python --version>nul 2>&1 && (
	echo Python environment found.
@roycaihwroycaihwJan 29, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
@Sai-AdarshSai-AdarshJan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
works fine without double qoute, so thought of reverting the change :)
@k8s-ci-robotk8s-ci-robot
added
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
cool.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 29, 2019

Copy link


Copy Markdown

Contributor
New changes are detected. LGTM label has been removed.
@k8s-ci-robotk8s-ci-robot
removed
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
@roycaihw could you check on the new changes?
@roycaihw
roycaihw
commented
Jan 29, 2019

Copy link


Copy Markdown

Member
Could you add the comments to the beginning of the file? Basically I was asking for a description so that people can tell what it's for and when it should be used. I think we don't need inline comments.
Something like "Install python client in Windows. Windows doesn't have symbolic link that this repo uses. This batch script provides a workaround for symbolic link by copying the folders over."
Also please squash your commits before submit
/hold cancel
@k8s-ci-robotk8s-ci-robot
removed
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@k8s-ci-robotk8s-ci-robot
added

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
and removed

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
labels
Jan 30, 2019
@micw523
micw523
commented
Feb 7, 2019

Copy link


Copy Markdown

Contributor
Will lg()tm after squash
@Sai-Adarsh
Sai-Adarsh
commented
Feb 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Will lg()tm after squash
cool.
@Sai-AdarshSai-Adarsh
mentioned this pull request
Feb 20, 2019
 Merged

@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 8, 2019
 Closed

Sai-Adarsh added a commit
to Sai-Adarsh/python
that referenced
this pull request
Mar 9, 2019
@Sai-Adarsh
Updating kubernetes-client owners with sai-adarsh
e18794c
Would like to join a GitHub team of kubernetes-client, and highly interested to join as a reviewer. Have previously merged a fix (kubernetes-client#734).
@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 9, 2019
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@roycaihwroycaihwroycaihw left review comments
@yliaogyliaogyliaog left review comments
@mbohloolmbohloolAwaiting requested review from mbohlool

+1 more reviewer

@micw523micw523micw523 requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@roycaihwroycaihw
@micw523micw523

Labels

cncf-cla: yesIndicates the PR's author has signed the CNCF CLA.size/SDenotes a PR that changes 10-29 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@Sai-Adarsh@k8s-ci-robot@micw523@roycaihw@yliaog

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fixes Setup.py error on Windows #676 - #734
Closed
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Closed
Fixes Setup.py error on Windows #676#734
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Conversation
@Sai-Adarsh
@Sai-AdarshSai-Adarsh
commented
Jan 21, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes Setup.py error on Windows, added shell script to check python environment, prints the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install issue #676
@Sai-Adarsh
Fixes Setup,py error on Windows#676
4a3c232
Fixes Setup,py error on Windows, added shell script to check python environment, print the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install
@k8s-ci-robotk8s-ci-robot
added
the 
size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
label
Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Sai-Adarsh

To fully approve this pull request, please assign additional approvers.

We suggest the following additional approver: roycaihw
If they are not already assigned, you can assign the PR to them by writing /assign @roycaihw in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment

Approvers can cancel approval by writing /approve cancel in a comment
@k8s-ci-robotk8s-ci-robot
added
the 
cncf-cla: yes
Indicates the PR's author has signed the CNCF CLA.
label
Jan 21, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 gave a PR. could you check this out.
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @roycaihw
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @micw523
@Sai-AdarshSai-Adarsh
changed the title
Fixes Setup,py error on Windows #676Fixes Setup.py error on Windows #676Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Sai-Adarsh: you cannot LGTM your own PR.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Appendly: changing LGTM is restricted to assignees, and only kubernetes-client/python repo collaborators may be assigned issues.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
yliaog
yliaog
reviewed
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

	echo Python environment found..
fi
python --version
cp -R ../kubernetes/base/* ../kubernetes/
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
"When we clone submodule with git the folders in the /kubernetes/base is supposed to be symbolically linked in the /kubernetes directory. In Windows this is not done. For a Windows developer, we’ll need to have a script copying all the base/* folders into */. "
^ is what @micw523 requested in his issue. so 'cp -R' does the job.
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,11 @@
if ! [ -x "$(command -v python)" ];
then 
	echo Python environment notfound..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

probably add double quotes around the echo string
Comment threadscripts/setup-fix.bat
Outdated

then 
	echo Python environment notfound..
else 
	echo Python environment found..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ditto
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ditto
cool. will do the necessary changes
@micw523micw523Jan 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

echo Python environment found..
echo Python environment found.
micw523
micw523
suggested changes
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

Comment threadscripts/setup-fix.bat
Outdated

	echo "Python environment notfound.."
else 
	echo "Python environment found.."
fi
@micw523micw523Jan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please check windows batch scripting syntax for if
@micw523
micw523
commented
Jan 22, 2019

Copy link


Copy Markdown

Contributor
To be honest this looks like bash instead of a Windows bat. You could probably use it in a Cygwin but I think Windows developers would prefer a genuine bat.
@k8s-ci-robotk8s-ci-robot
added

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
and removed

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
labels
Jan 24, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 done with the changes. could you check on the new commit :)
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,5 @@
( where python >null
	echo Python environment found..
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In this case Python environment found will be printed regardless.
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

python > null and found.
@Sai-AdarshSai-AdarshJan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

python > null and found.
cool. done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
done.
micw523
micw523
reviewed
Jan 28, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

See suggestion
@Sai-Adarsh
Sai-Adarsh
commented
Jan 28, 2019

Copy link


Copy Markdown

ContributorAuthor
See suggestion
done with changes.
@micw523
micw523
commented
Jan 28, 2019

Copy link


Copy Markdown

Contributor
/lgtm

/cc @roycaihw
@k8s-ci-robotk8s-ci-robot
added
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 28, 2019
roycaihw
roycaihw
reviewed
Jan 29, 2019
View reviewed changes
@roycaihwroycaihw
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,6 @@
(	python --version>nul 2>&1 && (
	echo Python environment found.
@roycaihwroycaihwJan 29, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
@Sai-AdarshSai-AdarshJan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
works fine without double qoute, so thought of reverting the change :)
@k8s-ci-robotk8s-ci-robot
added
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
cool.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 29, 2019

Copy link


Copy Markdown

Contributor
New changes are detected. LGTM label has been removed.
@k8s-ci-robotk8s-ci-robot
removed
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
@roycaihw could you check on the new changes?
@roycaihw
roycaihw
commented
Jan 29, 2019

Copy link


Copy Markdown

Member
Could you add the comments to the beginning of the file? Basically I was asking for a description so that people can tell what it's for and when it should be used. I think we don't need inline comments.
Something like "Install python client in Windows. Windows doesn't have symbolic link that this repo uses. This batch script provides a workaround for symbolic link by copying the folders over."
Also please squash your commits before submit
/hold cancel
@k8s-ci-robotk8s-ci-robot
removed
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@k8s-ci-robotk8s-ci-robot
added

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
and removed

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
labels
Jan 30, 2019
@micw523
micw523
commented
Feb 7, 2019

Copy link


Copy Markdown

Contributor
Will lg()tm after squash
@Sai-Adarsh
Sai-Adarsh
commented
Feb 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Will lg()tm after squash
cool.
@Sai-AdarshSai-Adarsh
mentioned this pull request
Feb 20, 2019
 Merged

@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 8, 2019
 Closed

Sai-Adarsh added a commit
to Sai-Adarsh/python
that referenced
this pull request
Mar 9, 2019
@Sai-Adarsh
Updating kubernetes-client owners with sai-adarsh
e18794c
Would like to join a GitHub team of kubernetes-client, and highly interested to join as a reviewer. Have previously merged a fix (kubernetes-client#734).
@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 9, 2019
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@roycaihwroycaihwroycaihw left review comments
@yliaogyliaogyliaog left review comments
@mbohloolmbohloolAwaiting requested review from mbohlool

+1 more reviewer

@micw523micw523micw523 requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@roycaihwroycaihw
@micw523micw523

Labels

cncf-cla: yesIndicates the PR's author has signed the CNCF CLA.size/SDenotes a PR that changes 10-29 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@Sai-Adarsh@k8s-ci-robot@micw523@roycaihw@yliaog

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Fixes Setup.py error on Windows #676 - #734
Closed
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Closed
Fixes Setup.py error on Windows #676#734
Sai-Adarsh wants to merge 14 commits into
kubernetes-client:masterfrom 
Sai-Adarsh:master
Conversation
@Sai-Adarsh
@Sai-AdarshSai-Adarsh
commented
Jan 21, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes Setup.py error on Windows, added shell script to check python environment, prints the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install issue #676
@Sai-Adarsh
Fixes Setup,py error on Windows#676
4a3c232
Fixes Setup,py error on Windows, added shell script to check python environment, print the python version, copy files from /kubernetes/base to /kubernetes/ and finally runs python setup.py install
@k8s-ci-robotk8s-ci-robot
added
the 
size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
label
Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Sai-Adarsh

To fully approve this pull request, please assign additional approvers.

We suggest the following additional approver: roycaihw
If they are not already assigned, you can assign the PR to them by writing /assign @roycaihw in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment

Approvers can cancel approval by writing /approve cancel in a comment
@k8s-ci-robotk8s-ci-robot
added
the 
cncf-cla: yes
Indicates the PR's author has signed the CNCF CLA.
label
Jan 21, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 gave a PR. could you check this out.
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @roycaihw
@Sai-Adarsh
Sai-Adarsh
commented
Jan 21, 2019

Copy link


Copy Markdown

ContributorAuthor
/assign @micw523
@Sai-AdarshSai-Adarsh
changed the title
Fixes Setup,py error on Windows #676Fixes Setup.py error on Windows #676Jan 21, 2019
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Sai-Adarsh: you cannot LGTM your own PR.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 21, 2019

Copy link


Copy Markdown

Contributor
@Appendly: changing LGTM is restricted to assignees, and only kubernetes-client/python repo collaborators may be assigned issues.
Details
In response to this:
/lgtm
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
yliaog
yliaog
reviewed
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

	echo Python environment found..
fi
python --version
cp -R ../kubernetes/base/* ../kubernetes/
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

what's the reason to do the 'cp -R' above?
"When we clone submodule with git the folders in the /kubernetes/base is supposed to be symbolically linked in the /kubernetes directory. In Windows this is not done. For a Windows developer, we’ll need to have a script copying all the base/* folders into */. "
^ is what @micw523 requested in his issue. so 'cp -R' does the job.
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,11 @@
if ! [ -x "$(command -v python)" ];
then 
	echo Python environment notfound..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

probably add double quotes around the echo string
Comment threadscripts/setup-fix.bat
Outdated

then 
	echo Python environment notfound..
else 
	echo Python environment found..
@yliaogyliaogJan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ditto
@Sai-AdarshSai-AdarshJan 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ditto
cool. will do the necessary changes
@micw523micw523Jan 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

echo Python environment found..
echo Python environment found.
micw523
micw523
suggested changes
Jan 22, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

Comment threadscripts/setup-fix.bat
Outdated

	echo "Python environment notfound.."
else 
	echo "Python environment found.."
fi
@micw523micw523Jan 22, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please check windows batch scripting syntax for if
@micw523
micw523
commented
Jan 22, 2019

Copy link


Copy Markdown

Contributor
To be honest this looks like bash instead of a Windows bat. You could probably use it in a Cygwin but I think Windows developers would prefer a genuine bat.
@k8s-ci-robotk8s-ci-robot
added

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
and removed

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
labels
Jan 24, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
@micw523 done with the changes. could you check on the new commit :)
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
I have a few comments -
  • Could you break your command into several lines? It is difficult to decipher at its current state
  • xcopy is considered an external program and has been deprecated since '07. Could you try using copy or robocopy?
done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,5 @@
( where python >null
	echo Python environment found..
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In this case Python environment found will be printed regardless.
@micw523micw523Jan 24, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

python > null and found.
@Sai-AdarshSai-AdarshJan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

python > null and found.
cool. done with the changes.
micw523
micw523
reviewed
Jan 24, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
@Sai-Adarsh
Sai-Adarsh
commented
Jan 24, 2019

Copy link


Copy Markdown

ContributorAuthor
Please fix your indents - the conditional statements are not aligned.

Also, one period for output message is sufficient.

On the last line ../ - the slash is not needed and on Windows you're supposed to use backslash anyway so cd .. should be good enough
done.
micw523
micw523
reviewed
Jan 28, 2019
View reviewed changes
@micw523micw523
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

See suggestion
@Sai-Adarsh
Sai-Adarsh
commented
Jan 28, 2019

Copy link


Copy Markdown

ContributorAuthor
See suggestion
done with changes.
@micw523
micw523
commented
Jan 28, 2019

Copy link


Copy Markdown

Contributor
/lgtm

/cc @roycaihw
@k8s-ci-robotk8s-ci-robot
added
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 28, 2019
roycaihw
roycaihw
reviewed
Jan 29, 2019
View reviewed changes
@roycaihwroycaihw
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
Comment threadscripts/setup-fix.bat
Outdated

@@ -0,0 +1,6 @@
(	python --version>nul 2>&1 && (
	echo Python environment found.
@roycaihwroycaihwJan 29, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
@Sai-AdarshSai-AdarshJan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

just out of curiosity: was the double quote change @yliaog suggested reverted for some reason?
works fine without double qoute, so thought of reverting the change :)
@k8s-ci-robotk8s-ci-robot
added
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
cool.
@k8s-ci-robot
k8s-ci-robot
commented
Jan 29, 2019

Copy link


Copy Markdown

Contributor
New changes are detected. LGTM label has been removed.
@k8s-ci-robotk8s-ci-robot
removed
the 
lgtm
"Looks good to me", indicates that a PR is ready to be merged.
label
Jan 29, 2019
@Sai-Adarsh
Sai-Adarsh
commented
Jan 29, 2019

Copy link


Copy Markdown

ContributorAuthor
/hold
Please add comments in the bat describing what it's for. Probably also name the file "windows-setup-fix.bat" to make it more clear. LGTM otherwise
@roycaihw could you check on the new changes?
@roycaihw
roycaihw
commented
Jan 29, 2019

Copy link


Copy Markdown

Member
Could you add the comments to the beginning of the file? Basically I was asking for a description so that people can tell what it's for and when it should be used. I think we don't need inline comments.
Something like "Install python client in Windows. Windows doesn't have symbolic link that this repo uses. This batch script provides a workaround for symbolic link by copying the folders over."
Also please squash your commits before submit
/hold cancel
@k8s-ci-robotk8s-ci-robot
removed
the 
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Jan 29, 2019
@k8s-ci-robotk8s-ci-robot
added

size/S
Denotes a PR that changes 10-29 lines, ignoring generated files.
and removed

size/XS
Denotes a PR that changes 0-9 lines, ignoring generated files.
labels
Jan 30, 2019
@micw523
micw523
commented
Feb 7, 2019

Copy link


Copy Markdown

Contributor
Will lg()tm after squash
@Sai-Adarsh
Sai-Adarsh
commented
Feb 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Will lg()tm after squash
cool.
@Sai-AdarshSai-Adarsh
mentioned this pull request
Feb 20, 2019
 Merged

@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 8, 2019
 Closed

Sai-Adarsh added a commit
to Sai-Adarsh/python
that referenced
this pull request
Mar 9, 2019
@Sai-Adarsh
Updating kubernetes-client owners with sai-adarsh
e18794c
Would like to join a GitHub team of kubernetes-client, and highly interested to join as a reviewer. Have previously merged a fix (kubernetes-client#734).
@Sai-AdarshSai-Adarsh
mentioned this pull request
Mar 9, 2019
 Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@roycaihwroycaihwroycaihw left review comments
@yliaogyliaogyliaog left review comments
@mbohloolmbohloolAwaiting requested review from mbohlool

+1 more reviewer

@micw523micw523micw523 requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@roycaihwroycaihw
@micw523micw523

Labels

cncf-cla: yesIndicates the PR's author has signed the CNCF CLA.size/SDenotes a PR that changes 10-29 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@Sai-Adarsh@k8s-ci-robot@micw523@roycaihw@yliaog