-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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] - Update build to support Installer #6827
Conversation
@ArthurSens , FYI, here is the draft PR for the Werft job changes. |
/werft run with-helm |
/werft run with-helm 👍 started the job as gitpod-build-kyleb-installer-werft.6 |
/status in-progress |
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-kyleb-installer-werft.55 |
/werft run 👍 started the job as gitpod-build-kyleb-installer-werft.63 |
/werft run 👍 started the job as gitpod-build-kyleb-installer-werft.64 |
/werft run with-helm with-clean-slate-deployment 👍 started the job as gitpod-build-kyleb-installer-werft.65 |
Codecov Report
@@ Coverage Diff @@
## main #6827 +/- ##
========================================
- Coverage 7.44% 5.76% -1.68%
========================================
Files 15 13 -2
Lines 1330 1162 -168
========================================
- Hits 99 67 -32
+ Misses 1228 1094 -134
+ Partials 3 1 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-kyleb-installer-werft.87 |
0c3cddb
to
1d6da80
Compare
ba6eba3
to
68819ae
Compare
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.
@mrsimonemms could I get a code review from you on the Go changes in this PR? They're mostly related to the Installer, and getting workloads to pass the PSP (which I bumped into in core-dev).
Labels: common.DefaultLabels(Component), | ||
Name: caIssuer, | ||
Labels: common.DefaultLabels(Component), | ||
Namespace: ctx.Namespace, |
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.
kubectl delete
was throwing an error w/o the Issuer being namespaced.
fi | ||
|
||
echo "Removing Gitpod in namespace ${NAMESPACE}" | ||
kubectl get configmap gitpod-app -n "${NAMESPACE}" -o jsonpath='{.data.app\.yaml}' | kubectl delete -f - |
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 shellscript is necessary because the kubectl delete
was getting a no resources found in the TypeScript file. In other words, this is a workaround.
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.
Understood. Would be awesome if the installer had such a delete
feature itself.
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.
Hey @geropl what would the use case be for that? I ask because we also have with-clean-slate-deployment=true
- which removes either Helm or Installer, and then installs with the Installer if no with-helm
was specified. If with-helm=true
was specified, it'll clean then install with Helm.
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.
Or do you mean have the installer connect to the Kubernetes installation, and remove Gitpod itself?
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.
Or do you mean have the installer connect to the Kubernetes installation, and remove Gitpod itself?
Sorry for not being clear: yes. IMO the installer should be able to completly remove everything it added. We already have the gitpod-app
Configmap which contains everything required (although only for the last render
run).
Thank you for the approval, @mads-hartmann ! Let's leave the hold applied and wait to hear back from Team Meta. @gitpod-io/engineering-meta may we ask for a review from you as well? You'll notice there is an "escape hatch", where you can continue to use |
Also, once the meta review is finished, I would like to squash this PR, so let's leave the hold until then. 🙏 |
@kylos101 Starting to review now 👀 |
const CONTAINERD_RUNTIME_DIR = "/var/lib/containerd/io.containerd.runtime.v2.task/k8s.io"; | ||
|
||
// get some values we need to customize the config and write them to file | ||
exec(`yq r ./.werft/values.dev.yaml components.server.blockNewUsers \ |
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 understand that this is a concession to the interface problem around values.dev.yaml
for as long as we have two ways to install on devstaging. I really hope we can get rid of this, soon. :slighty_smiling_face:
if [[ "server-config" == "$NAME" ]] && [[ "$KIND" == "ConfigMap" ]]; then | ||
WORK="overrides for $NAME $KIND" | ||
echo "$WORK" | ||
touch /tmp/"$NAME"overrides.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.
After reading in detail, this is less stuff than I feared. It's a bit noisy (not necessarily the bad kind) but most of it is connected to the devstaging setup. 👍
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.
Nice PR @kylos101! Thx for all the explanation :-)
Keeping the hold
in case you want to time/announce the merge.
5705e38
to
6ceb88e
Compare
Hi @mrsimonemms and @iQQBot , I had to do a follow-up commit which removed the L G T M label. May I ask for a follow-up code review so we can add L G T M label back? Also, please leave the hold label on, as I haven't squashed yet. Please review:
And if you approve, then we can add the L G T M label back. Thank you for your help! |
Hey. Awesome to this about to be merged - excellent effort everyone! This is a lot of commits (43). @kylos101 could you please go through and squash commits so that the remaining commit history makes sense once it's on main? |
The Installer is the default for new Preview Environments. Preview Environments powered by Helm can switch to the Installer if 'with-clean-slate-deployment=true' is specified. Folks can continue using Helm if 'with-helm=true' is specified. Co-authored-by: Christian Weichel <chris@gitpod.io>
426f82a
to
7ac89a8
Compare
Hey @csweichel all set in 7ac89a8. Helm build here (which is very much like anyone currently using Helm will see): I'll do a follow-on where it removes the Helm install, and then uses the Installer, then we should be 🟩 . |
/werft run with-clean-slate-deployment=true 👍 started the job as gitpod-build-kyleb-installer-werft.223 |
/lgtm |
LGTM label has been added. Git tree hash: 481c6b9fe7291e35530751d728c5e0a86bae1a94
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csweichel, geropl, kylos101, mads-hartmann Associated issue: #6808 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/unhold
|
Description
[Werft] - Update build to support Installer
Related Issue(s)
Fixes #6808
How to test
werft run
(runs Installer) orwerft run with-helm
(runs helm)werft run with-clean-slate-deployment
(will do clean-up, even for helm, and then install with Installer)werft run with-clean-slate-deployment with-helm
(will do clean-up, even for Installer, and then run Helm)werft run ws-feature-flags=registry-facade
(sets flag)with-observability
andwith-payment
have not been implemented, if needed please usewith-helm
too.Meta considerations
Implemented:
Not implemented:
with-payment
integration for chargebeesRelease Notes
Documentation