-
Notifications
You must be signed in to change notification settings - Fork 163
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
New development environment + setup document #1507
Conversation
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.
Thanks for the PR, the work here will help standardize our development flows
Overall LGTM, just left a couple questions
Thanks @wajihyassine. I have made the changes and added some extra explanation in the comment and document regarding hot reloading. Let me know if that makes more sense now (and resolve the comment). |
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.
LGTM - note that you can use the reload
parameter with uvicorn to provide hot reloading for the API server too.
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.
LGTM
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.
Cool, looking forward to this. Just a few comments.
### Configure VSCode | ||
Start VSCode and install the following extensions: | ||
* [Python](https://marketplace.visualstudio.com/items?itemName=ms-python.python) | ||
* [Gemini Code Assist + Google Cloud Code](https://marketplace.visualstudio.com/items?itemName=GoogleCloudTools.cloudcode) |
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.
So is this only needed when doing development in GCE then? I can't seem to load the link here right now so not sure how these pieces fit together, but is the Gemini code assist part required?
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.
No, the extension will manage all the dependencies (among others: minikube, kubectl and skaffold) for you independent if your machine is a local or a remote GCE. It will install and configure the dependecies in a way that do not interfere with your machine configuration.
The Gemini code assist is included as of lately (unfortunatly..?) and is not activated by default.
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.
Interesting, so the only thing we're using it for is dependency management and don't actually use for anything cloud? What form are all those dependencies in, are they just vscode extensions or are there other packages it installs too? Is there maybe a lighter weight or better way to do that, or is that just easiest?
Could we maybe add a note clarifying exactly what it's used for (and not used for, e.g. actual cloud management) and that Gemini is not used or enabled by default?
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.
The exension manages minikube, skaffold, kubectl and their setup and configuration with a single Install click of the extension. I can't think of an easier way. It will also install these dependencies in a seperate folder so it will not interfere with your system.
The extension will be using the Cloud component for the GKE development option to be added in a further PR.
Gemini code assist is not enabled by default, added a note.
The extension and its use is described in more details at the top of this document in the Components section.
## Setup | ||
### Requirements | ||
Please install the following requirements on your system. | ||
* [Docker](https://docs.docker.com/engine/install/), you can install Docker Desktop or only the engine. |
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 might be misunderstanding how the GCP parts of this work, but if these are not required in that case, can we add a note for which requirements are needed whether you are doing this locally vs. remote?
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.
They are all required. This document describes a non-GKE setup, so a local development setup. Local can be seen here as your own laptop or a GCE machine (which is considered a local machine in the cloud...lol).
The remote k8s development option running on GKE will be added in follow-up PRs.
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 see, makes sense. Could we maybe add a note about this too to make it clear how we're using the GCE instance with minikube there locally?
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.
Added note to the introduction section where the GCE machine is mentioned.
--- | ||
NOTE: This setup has been tested and is in active use by the Turbinia developers in the following configurations. | ||
* MacAir M3 8GB with Docker Desktop | ||
* GCE (e2-standard-4) Debian 12/bookworm 16GB with Docker Engine (Running on GCP using the [VSCode Remote SSH extension](https://marketplace.visualstudio.com/items?itemName=ms-vscode-remote.remote-ssh)) |
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.
How does this work with GCE then, is minikube still deployed locally on the GCE instance? Are there any restrictions or special config to do things like attaching GCP disks?
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.
Yes, the GCE machine should be seen as a local machine in the cloud. The attaching of GCP disks is only possible in Turbinia when running under GKE due to the fact on how we connect disks to Nodes instead of Pods. The GKE development option will be added in a follow-up PR.
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 think attaching to a GCE instance should actually work, but not sure how running under minikube might change that. I think we'd need to map in /dev
if we're not already. AFAIK we actually attach to the node and not the pod which is why we need the resource tracking code so we don't detach something already attached when another worker/node is processing it.
turbinia/turbinia/processors/google_cloud.py
Line 106 in 1617dcc
instance = project.compute.GetInstance(instance_name) |
Line 572 in 1617dcc
is_detachable = resource_manager.PostProcessResourceState( |
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.
Could be but I prefer a standard setup on all machines/environments for development and not customize it for different machines. If you want to develop by processing GCE disks the GKE development option is the way to go.
Folks, thanks for the reviews! @aarontp Thanks for the comments, I have answered them. Feel free to reply and/or resolve if all clear. |
LGTM, maybe with a couple clarifying comments as mentioned above. Thanks! |
@aarontp thanks for the good feedback, added some notes and explanations to the document based on your feedback. PTAL. |
Description of the change
This PR contains
Applicable issues
Additional information
None
Checklist