-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove dependency on git from prepare node script #280
Conversation
Welcome @jonaskello! |
@jsturtevant I put the gitpath in a variable and made it point to the latest version. It would be better if it pointed to the exact version but I don't see how that could be done without depending on cloning the repo and using git. |
kubeadm/scripts/PrepareNode.ps1
Outdated
Write-Output "Please remember that after you have joined the node to the cluster, that you have to apply the cni daemonset/service and the kube-proxy" | ||
Write-Output "Also remember that for kube-proxy you have to change the its version from the name of the image in the kube-proxy.yml to that of your kubernetes version `n" | ||
# rancher commands | ||
Write-Output "In case you use rancher, use the following commands:" | ||
Write-Output "For Windows you can use the following command: " | ||
Write-Output "(Get-Content `"$(git rev-parse --show-toplevel)/kubeadm/kube-proxy/kube-proxy.yml`") -Replace 'VERSION', '$KubernetesVersion' | Set-Content `"$(git rev-parse --show-toplevel)/kubeadm/kube-proxy/kube-proxy.yml`" `n" | ||
Write-Output "(Get-Content `"$gitpath/kubeadm/kube-proxy/kube-proxy.yml`") -Replace 'VERSION', '$KubernetesVersion' | Set-Content `"$gitpath/kubeadm/kube-proxy/kube-proxy.yml`" `n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this right?
Set-Content `"$gitpath/kubeadm/kube-
I would have thought this would be local path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I misunderstood the original intention here I think. I think this part of the script assumes that you git cloned the whole repo down.
However the guide tells you to download this script with curl and run it. So in that case it will not run from within a cloned repo and local paths will not work. So for that reason I think the commands to apply the manifests for kubeproxy should refer to the online manifests by using http urls.
However then of course the Get-Content
needs to be changed to curl
commands. I'll do that and see if it makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up first downloading the manifest and then patching the local file with the version. Do you think this will work?
kubeadm/scripts/PrepareNode.ps1
Outdated
@@ -105,24 +105,26 @@ nssm set kubelet DependOnService containerd | |||
|
|||
New-NetFirewallRule -Name kubelet -DisplayName 'kubelet' -Enabled True -Direction Inbound -Protocol TCP -Action Allow -LocalPort 10250 | |||
|
|||
$gitpath='https://raw.githubusercontent.com/kubernetes-sigs/sig-windows-tools/master' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gitpath varaible is a bit misleading. maybe repositoryRoot or something else?
Thinking some more on this, perhaps the last part of the script that prints instructions on how to setup kube-proxy could be removed and the instructions moved to the guide document instead? |
yea this makes sense to me... I think it might already be in those guides? This was added before we had a guide. |
/lgtm |
LGTM, but needs a rebase. |
New changes are detected. LGTM label has been removed. |
Should be rebased now |
@jonaskello Could you squash the commits and make sure the commits are valid? (I am not really sure what is invalid about them but hoping squashing will get rid of the warning |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jonaskello, jsturtevant The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@jsturtevant Failed to fix this PR, making a new attempt in #299 |
Reason for PR:
Eliminates dependency on git command
Issue Fixed:
278
Requirements