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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preview environment CLI #10076

Merged
merged 4 commits into from
May 31, 2022
Merged

Preview environment CLI #10076

merged 4 commits into from
May 31, 2022

Conversation

ArthurSens
Copy link
Contributor

@ArthurSens ArthurSens commented May 17, 2022

Description

Following our internal RFC, the work done in this CLI was to make sure it is easily extendable with new commands and that the architecture makes it easy to test.

It also introduces the first command asked in Milestone 1(see important notes) of that same RFC. Which makes it possible to install the kube-context of different preview environments.

Important notes

install-k3s-context.sh was not re-implemented in Go yet.

The implementation is not easy and I think it would delay this PR too much. Our current implementation in bash uses SSH tunneling and I'm not sure how to implement/replace that in Go. I'm also not sure how to write a good test for such implementation 馃槵.

If possible, I'd like to convert this into a separate issue and tackle the re-implementation and testing into a separate PR.

install-k3s-context.sh was modified to install context from custom branches.

Since one of the requirements was the ability to install context from different branches, I extended our bash script to install context from custom branches 馃檪

install-k3s-context.sh doesn't exit with code != 0 when preview doesn't exist.

And as a consequence, previewctl install-context <unexisting-branch> succeeds even with a broken context. I'm on the fence if this bug should be a blocker for this PR or not, but I've already spent a good time trying to solve this one without success 馃槄. The options that I see are:

  • Since users of this CLI are mostly interested in their own previews(which works), this bug could be tackled in a separate issue/PR.

Or

  • We shouldn't merge this PR until the bug is fixed, therefore I need some extra help 馃槤

Code walkthrough

https://www.loom.com/share/6da6cf3b6b61457e8c0f17736e716434

How to test

A couple of options to try

  • Start a workspace from this pull request - The right context should be already installed
  • Try installing the context from another existing branch by running previewctl install-context --branch <branch-name>
  • Create a new branch from this PR, commit something, open a new workspace from the new branch/commit, the context should be installed when starting the workspace.

Release Notes

NONE

@ArthurSens
Copy link
Contributor Author

Pinging @gitpod-io/platform just for awareness and in case you want to provide early feedback

@roboquat roboquat added size/XL and removed size/L labels May 25, 2022
@ArthurSens ArthurSens force-pushed the as/previewctl branch 8 times, most recently from 0a34f68 to 4674a75 Compare May 26, 2022 20:01
@ArthurSens ArthurSens marked this pull request as ready for review May 26, 2022 20:34
@ArthurSens ArthurSens requested a review from a team May 26, 2022 20:34
@ArthurSens ArthurSens changed the title WIP: Preview environment CLI Preview environment CLI May 26, 2022
@vulkoingim
Copy link
Contributor

Looks good, I just have a few nits about the file names in the cmd package - in general the convention is not to use mixed caps in the file/package names. Usually I would go with a single lowercase word.

@ArthurSens
Copy link
Contributor Author

Looks good, I just have a few nits about the file names in the cmd package - in general the convention is not to use mixed caps in the file/package names. Usually I would go with a single lowercase word.

Ah yes, thanks for pointing that out. Just renamed the files :)

Comment on lines 1 to 4
/*
Copyright 漏 2022 NAME HERE <EMAIL ADDRESS>

*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This is in a few places and is unnecessary :) Unless there's actually a requirement to fill it with something relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh whoops, that was auto-generated and I forgot to clean it up 馃槄. Thanks!

It is required indeed, but not like that :P

@ArthurSens
Copy link
Contributor Author

/hold

Looks like some changes made to main have broken this build :/, I'll try rebasing

Following our [internal RFC](https://www.notion.so/gitpod/A-Go-based-CLI-for-interacting-with-Preview-Environments-1834aa90bc104a0b836dd523e22f9e93), the work done in this CLI was to make sure it is easily extendable with new commands and that the architecture makes it easy to test.

It also introduces the first command asked in Milestone 1 of that same RFC. Which makes it possible to install the kube-context of different preview environments.

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>
@github-actions
Copy link
Contributor

鈿狅笍 Hey reviewer! BE CAREFUL 鈿狅笍
Review the code before opening in your Gitpod. .gitpod.yml was changed and it might be harmful.

@ArthurSens
Copy link
Contributor Author

It did the trick :)
/unhold

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants