-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
.werft: Refactoring to use constant kubeconfig files instead of overrides #9016
Conversation
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
/werft run with-vm 👍 started the job as gitpod-build-arthursens-refactor-deploy-to-preview-8997.8 EDIT: This got stuck and timed-out, I'm looking into it |
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.
This is really impressive Arthur 👏 I left a few comments, but I don't think either of them are blocking. The biggest one is about quoting the paths we pass to exec. As we have full control over the paths in our source code I don't think it's worth making the change but I'll leave it up to you to decide.
I added the blocked label so you can test all the various scenarios.
@@ -7,7 +7,7 @@ import * as fs from 'fs'; | |||
* Monitoring satellite deployment bits | |||
*/ | |||
export class InstallMonitoringSatelliteParams { |
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.
This would have been nicer to just have as a type
instead of class.
} | ||
|
||
async function ensureCorrectInstallationOrder(namespace: string, checkNodeExporterStatus: boolean){ | ||
async function ensureCorrectInstallationOrder(kubeconfig: string, namespace: string, checkNodeExporterStatus: boolean){ |
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.
For this file you could have exposed a single class and then have all the functions as public/private methods. That way you wouldn't need to pass the kubeconfig around.
I don't think it's worth doing now, just as inspiration for future refactoring if you're feeling like it 😉
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.
Yeah, that definitely crossed my mind while doing this, maybe its worth raising an issue to refactor this later
@@ -99,18 +99,18 @@ export async function deployToPreviewEnvironment(werft: Werft, jobConfig: JobCon | |||
withVM, | |||
}; | |||
|
|||
exec(`kubectl --namespace keys get secret host-key -o yaml > /workspace/host-key.yaml`) | |||
exec(`kubectl --kubeconfig ${CORE_DEV_KUBECONFIG_PATH} --namespace keys get secret host-key -o yaml > /workspace/host-key.yaml`) |
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.
To be on the 100% safe side it would be nicest to quote the path, so it is --kubeconfig "${CORE_DEV_KUBECONFIG_PATH}"
. This goes for all of the places where we are using a path in an exec
call
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
/werft run with-vm 👍 started the job as gitpod-build-arthursens-refactor-deploy-to-preview-8997.27 EDIT: Problems copying certificates from core-dev to the preview on k3s |
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
/werft run with-vm |
/werft run with-helm 👍 started the job as gitpod-build-arthursens-refactor-deploy-to-preview-8997.32 EDIT: Failed to clean previous installer installation prior to deploy with helm |
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-arthursens-refactor-deploy-to-preview-8997.35 |
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
/werft run with-clean-slate-deployment with-helm 👍 started the job as gitpod-build-arthursens-refactor-deploy-to-preview-8997.37 |
Oh damn, I spent way too much time trying to understand how my changes could have made helm to fail, but it is also failing in |
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-arthursens-refactor-deploy-to-preview-8997.38 |
Tested with installer on both core-dev and Harvester, worked fine. Helm was a failure 🙅, but, after testing on main, looks like helm is currently broken everywhere :/ |
…redentials Signed-off-by: ArthurSens <arthursens2005@gmail.com>
/werft run with-helm 👍 started the job as gitpod-build-arthursens-refactor-deploy-to-preview-8997.41 |
Sorry team-webapp, I'm just cherrypicking Janx's commit to test a deployment with Helm, I'll drop the commit afterwards |
8586719
to
540ada2
Compare
Description
This PR refactors the whole deployment job so all interactions with any cluster have to be made with the appropriate kubeconfig file explicit set. This is being done because we've been constantly overriding kubecontexts throughout the job's logic and we've got to a point where it is super confusing to know what is being applied where.
It is a huge change and too much effort to review only looking at the changes. I'm making sure it works by starting multiple jobs using different configurations and checking if Gitpod runs and is able to start workspaces afterwards.
Related Issue(s)
Fixes #8997
How to test
Release Notes