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

Update README.md #92

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Update README.md #92

wants to merge 5 commits into from

Conversation

TomAugspurger
Copy link
Member

I believe these docs were out of date.

@jacobtomlinson
Copy link
Member

Thanks @TomAugspurger.

We use Frigate here for autogenerating the README. The main benefit is that it keeps the table of configuration options up to date. Would you mind updating the template and regenerating the REAMDE?

https://github.com/dask/helm-chart#generating-the-readme

(I made some changes to Frigate this week which will make it super useful for daskhub as it will pull all the possible configuration options from the dependency charts too)

@jacobtomlinson
Copy link
Member

This whole process of using a template and generating the README from it is a little annoying. Frigate also has a sphinx plugin so we may just want to move the docs to RTD instead of the README to make the whole experience more pleasant.

dask/README.md Outdated
@@ -65,7 +65,7 @@ The following table lists the configurable parameters of the Dask chart and thei
| `scheduler.image.pullPolicy` | Container image pull policy. | `"IfNotPresent"` |
| `scheduler.image.pullSecrets` | Container image [pull secrets](https://kubernetes.io/docs/tasks/configure-pod-container/pull-image-private-registry/). | `null` |
| `scheduler.replicas` | Number of schedulers (should always be 1). | `1` |
| `scheduler.serviceType` | Scheduler service type. set to `loadbalancer` to expose outside of your cluster. | `"ClusterIP"` |
| `scheduler.serviceType` | Scheduler service type. set to `LoadBalancer` to expose outside of your cluster. | `"ClusterIP"` |
Copy link
Member

Choose a reason for hiding this comment

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

This may be a bug in Frigate because the comment in the values.yaml this is generated from looks correct.

https://github.com/dask/helm-chart/blob/master/dask/values.yaml#L14

Copy link
Member Author

Choose a reason for hiding this comment

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

If I had to guess it's https://github.com/rapidsai/frigate/blob/874493d8fbfd8c7fe94f13a709bcf7bf92cc680b/frigate/gen.py#L184.

I think it'd be best if frigate didn't try to be clever here. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think dropping the capitalise call would be best.

TomAugspurger added a commit to TomAugspurger/frigate that referenced this pull request Aug 25, 2020
As noted in dask/helm-chart#92 (comment),
in some comments capitalization is important so frigate shouldn't change
it.
jacobtomlinson pushed a commit to rapidsai/frigate that referenced this pull request Aug 25, 2020
As noted in dask/helm-chart#92 (comment),
in some comments capitalization is important so frigate shouldn't change
it.
@consideRatio consideRatio added the chart/dask Related to the dask chart label Nov 6, 2020
Base automatically changed from master to main February 11, 2021 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chart/dask Related to the dask chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants