Skip to content

Refactor previewctl install-context #14241

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

Merged
merged 1 commit into from
Nov 3, 2022
Merged

Refactor previewctl install-context #14241

merged 1 commit into from
Nov 3, 2022

Conversation

vulkoingim
Copy link
Contributor

@vulkoingim vulkoingim commented Oct 27, 2022

Description

  • Changes the underlying implementation of the previewctl install-context command from bash to go and adds tests
  • Changes parts of the get-credentials command, that was introduced recently so it fits better with the new design and adds tests to some of the functions as well
  • It doesn't change how preivewctl is used and the context is installed in workspaces yet (follow up pr will deal with that)

Related Issue(s)

Fixes https://github.com/gitpod-io/ops/issues/5763

How to test

gcloud auth login

cd dev/preview/previewctl 

KUBECONFIG=$(pwd)/tmp go run main.go install-context

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@vulkoingim vulkoingim changed the title Aa/k3s install Refactor previewctl install-context Oct 28, 2022
@vulkoingim vulkoingim force-pushed the aa/k3s-install branch 7 times, most recently from b71efcb to c88a42a Compare November 2, 2022 19:40
@vulkoingim vulkoingim marked this pull request as ready for review November 2, 2022 19:41
@vulkoingim vulkoingim requested a review from a team November 2, 2022 19:41
Copy link
Contributor

@mads-hartmann mads-hartmann left a comment

Choose a reason for hiding this comment

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

This is awesome Aleks. I left a few inline comments.

For a big PR like this I would have loved a PR description that provided more context about the change. Here are a few questions I have that would have been nice to have in the PR description.

  1. We'll have to bump the version of previewctl in the image (unfortunately). This isn't done in this PR so I'm assuming you'll do it in a follow-up PR.
  2. With this change we should be able to delete this
  3. Install-contexts isn't used in any of the Werft jobs yet. What's the plan for using it? It would be nice to be able to delete this and switch any invocations of it like this to use install-context. We should also be able to simplify the Werft job by removing the stateful handling of port-forwarding that install-context now takes care of (e.g. removing this and related code)

@vulkoingim
Copy link
Contributor Author

vulkoingim commented Nov 3, 2022

For a big PR like this I would have loved a PR description that provided more context about the change. Here are a few questions I have that would have been nice to have in the PR description.

Yeah I agree - I just marked it ready for review when I was mostly done with it yesterday and left the description for today. Didn't think that anyone will review it that soon 😄

  1. We'll have to bump the version of previewctl in the image (unfortunately). This isn't done in this PR so I'm assuming you'll do it in a follow-up PR.

Yep

  1. With this change we should be able to delete this

Also yes

It would be nice to be able to delete this

Will remove this eventually

switch any invocations of it like this

Will change this later as well

We should also be able to simplify the Werft job by removing the stateful handling of port-forwarding that install-context now takes care of (e.g. removing this and related code)

Yeah, we should be able to remove this and the part that copies the context in the job also.

Edit: Issue for ☝️

Copy link
Contributor

@mads-hartmann mads-hartmann 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 7b4377a into main Nov 3, 2022
@roboquat roboquat deleted the aa/k3s-install branch November 3, 2022 12:30
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.

3 participants