-
Notifications
You must be signed in to change notification settings - Fork 89
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
[helm] Initial Helm chart #950
Conversation
Helm install (from commit 7da7b3e) successfully applied to aws and google deployments. |
Closed according to the new tsc contributing guidelines. |
This first version of the helm chart is a partial migration of the tanka manifests. The following parts are missing:
|
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.
Pretty advanced stuff 👍 I was surprised at first that your values.schema.json
also includes values of the cockroachDB helm chart, but then I realized that you also use some of these values. I tried rendering that helm chart locally, I didn't try deploying it though, but it LGTM
Tested successfully as part of a CI run: https://github.com/Orbitalize/dss/actions/runs/7398206495/job/20126882086#step:6:10354 CI code and updates to terraform required to deploy the helm chart will be the subject of another PR. #983 |
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.
Looks good to me, though I am probably not the most ideal technical reviewer. I have some comments regarding the documentation, but your discretion whether they are addressed for this PR or in a separate PR.
@@ -1,11 +1,13 @@ | |||
# DSS Deployment | |||
|
|||
**Work in progress** | |||
|
|||
This folder contains the increments toward the new deployment approach as described in [#874](https://github.com/interuss/dss/issues/874). | |||
|
|||
The infrastructure folder contains the terraform modules to deploy the DSS to kubernetes clusters of various cloud providers: |
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.
Is this still true? Isn't the infrastructure folder supposed to simply set up the Infrastructure to be ready to accept Services deployment? If that is the case, it seems like DSS deployment requires Infrastructure + Services and it would be more appropriate here to say something like "The infrastructure folder contains the terraform modules to prepare infrastructure on various cloud providers to accept deployment of Services below". If that's not the case, what is the difference between Infrastructure and Services, and why is Services needed at all if Infrastructure fully deploys the DSS? Perhaps the infrastructure folder actually contain Infrastructure + Services-via-not-Helm?
It seems like it would likely be valuable to answer in the form of transferring some of the documentation from #874 to this file and/or the Infrastructure and Services READMEs (especially the definitions of Infrastructure, Services, and Operations)
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.
@@ -1,11 +1,13 @@ | |||
# DSS Deployment | |||
|
|||
**Work in progress** | |||
|
|||
This folder contains the increments toward the new deployment approach as described in [#874](https://github.com/interuss/dss/issues/874). | |||
|
|||
The infrastructure folder contains the terraform modules to deploy the DSS to kubernetes clusters of various cloud providers: |
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.
To facilitate navigation solely from the README content, it would probably be nice to enclose "infrastructure folder" and "service folder" in links to those respective folders. The links are immediately available when the README is displayed as a result of viewing the folder in GitHub (basically using it as an index.html), but they're not immediately available when viewing the README specifically.
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.
#984 regorganizes the documentation and this file now encloses all modules.
@@ -0,0 +1,5 @@ | |||
# Services | |||
|
|||
This folder contains the tools to deploy a DSS to a Kubernetes cluster. |
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 relate to Deployment of the DSS services included in the infrastructure
section of deployment? When would a user use one versus the other, and how would they know to do that?
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 file has been removed in #984 in favour of a consolidated description of all modules.
Thanks for the comments. I will merge the open PRs and consolidate the documentation in a new one based on your feedback as you suggested. |
Progressing on the objective #874, this PR adds the ability to deploy the DSS using helm to a Kubernetes cluster.
It was tested on AWS and Google deployment using the terraform scripts to deploy the infrastructure.
A future PR will update the terraform-commons-dss to automatically deploy the DSS using this helm chart.
Few remarks: