Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove core-dev preview environment options #10795

Merged
merged 2 commits into from
Jun 28, 2022

Conversation

liam-j-bennett
Copy link
Contributor

@liam-j-bennett liam-j-bennett commented Jun 21, 2022

Description

Removes the core-dev preview environment options

Related Issue(s)

Fixes #2529

Release Notes

NONE

Werft options:

  • /werft with-preview

@liam-j-bennett
Copy link
Contributor Author

/release-note-none

@meysholdt
Copy link
Member

hi @liam-j-bennett!
It's great to see how much simpler our code becomes - can you remove core-dev-related code from the cleanup job as well? see here.

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-ljb-2529-remove-core-dev-preview-env.1 because the annotations in the pull request description changed
(with .werft/ from main)

Copy link
Contributor

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Nice clean-up 🧹!

Just like mo said, I believe we can clean up even more! I think we can also un-mount some secrets from .werft/build.yaml and also delete some terraform code that creates those secrets(terraform would be another PR in another repo of course 🙂)

.werft/jobs/build/prepare.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

LGTM!

I see that we have a hold label in place, so I'm already approving because the PR solves the problem: "Discontinue previews in core-dev" 🙂

Just wanted to remind the team of another problem that we still have which is getting rid of pieces of infrastructure required to run previews in core-dev. Things that come to my mind:

  • Secrets in .werft/build.yaml
  • Terraform code for those same secrets

But there might be more 😬

Copy link
Member

@meysholdt meysholdt left a comment

Choose a reason for hiding this comment

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

This looks great already! I found one line that looks like it should be deleted, too and two nits. See comments.

.werft/observability/monitoring-satellite.ts Show resolved Hide resolved
.werft/util/kubectl.ts Outdated Show resolved Hide resolved
Copy link
Member

@meysholdt meysholdt left a comment

Choose a reason for hiding this comment

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

🚀 🚀 🚀

@liam-j-bennett liam-j-bennett force-pushed the ljb/2529-remove-core-dev-preview-env branch from 1b8131c to 1cb8fce Compare June 28, 2022 08:42
@meysholdt
Copy link
Member

Thx for updating the PR! The code changes LGTM, but I see the build failing via:

Error Error: kubectl --kubeconfig /workspace/gitpod/kubeconfigs/k3s apply -f k8s.yaml exit with non-zero status code.
The Job "migrations" is invalid: spec.template: Invalid value:... (there are more details in the log)

Copy link
Member

@meysholdt meysholdt left a comment

Choose a reason for hiding this comment

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

After taking a second look, I think two of the secrets are still in use.

.werft/build.yaml Show resolved Hide resolved
.werft/build.yaml Outdated Show resolved Hide resolved
@liam-j-bennett
Copy link
Contributor Author

Regarding the build error:

        this.options.werft.log(slice, "Installing Gitpod");
        exec(`kubectl --kubeconfig ${this.options.kubeconfigPath} delete -n ${this.options.deploymentNamespace} job migrations || true`, { silent: true });
        // errors could result in outputing a secret to the werft log when kubernetes patches existing objects...
        exec(`kubectl --kubeconfig ${this.options.kubeconfigPath} apply -f k8s.yaml`, { silent: true });

Is the delete failing somehow? We ignore all errors, not just the NotFound

@liam-j-bennett liam-j-bennett force-pushed the ljb/2529-remove-core-dev-preview-env branch from 1bd132e to d159f29 Compare June 28, 2022 09:22
@liam-j-bennett liam-j-bennett force-pushed the ljb/2529-remove-core-dev-preview-env branch from d159f29 to 8e1e733 Compare June 28, 2022 09:23
@liam-j-bennett
Copy link
Contributor Author

Build issue seems to have fixed itself

Copy link
Member

@meysholdt meysholdt left a comment

Choose a reason for hiding this comment

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

🚀 🚀 🚀

@roboquat roboquat merged commit 4f0619e into main Jun 28, 2022
@roboquat roboquat deleted the ljb/2529-remove-core-dev-preview-env branch June 28, 2022 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants