ci, pre-commit: rely on pre-commit.ci service to run tests

[consideRatio]

dask/community#222

UPDATE: For frigate to run and generate daskhub/README.md, it seems it must first do helm dep up to download dependency Helm charts. But, pre-commit.ci can't be configured to do that first I think. So, I think we can't use pre-commit.ci.

Action points

• Activate pre-commit.ci as a service for this project.
  This is done by visiting the pre-commit.ci GitHub app and making it available for the dask GitHub organization to use: https://results.pre-commit.ci/
  And declare it to be used specifically on this github repository at: https://github.com/organizations/dask/settings/installations
  This means it would...
  a) automatically runs a pre-commit CI/CD check like a github workflow would
  b) automatically create PRs updating the version of pre-commit hooks defined in .pre-commit-config.yaml when new versions are out - at most once a week.
  c) automatically add commits to PRs with autoformatting changes based on the .pre-commit-config.yaml file if needed
• Merge this PR if accepted

[jacobtomlinson]

Perhaps this is a fix we need to make in frigate?

[consideRatio]

@jacobtomlinson yepp absolutely - that would be great! If you get that setup, it would be the last time you had to manually add commits or wait for people to add them to PRs after having made a change that isn't updated. Having pre-commit.ci push a commit for such changes would make perfect sense I think.

[jacobtomlinson]

Circling back here because if we had pre-commit CI then #274 would be passing.

The problem here seemed to be around chart dependencies not being up to date. It looks like frigate does already update dependencies so I'm not entirely sure why we are also doing it in the existing action. But I don't expect it to be a problem to drop it. Unless there is a bug in Frigate that I'm not aware of that we are working around here.

https://github.com/rapidsai/frigate/blob/762e75b56fb071d30cf0823c341747a738db1dd2/frigate/gen.py#L98

[consideRatio]

Hmmm, I'm not so read up on how frigate works or does, but I can conclude that README.md includes details about the JupyterHub Helm chart's default values.yaml, which means that frigate has consumed the charts/ directory after helm dep up was run to fetch the declared dependencies in Chart.yaml.

Also, there is a coupling to the jupyterhub helm chart about values in the daskhub helm chart somehow, I think what happens is that the daskhub helm chart imports values from the dependency chart. If that is done, it would also make sense that the default values of daskhub would require jupyterhub to be around to provide default values.

helm-chart/daskhub/Chart.yaml

Lines 11 to 13 in b09f9b2

	import-values:
	- child: rbac
	parent: rbac

Overall, I think the practice of generating these kind of reports in the markdown document alongside values.yaml and alongside a values.schema.json file that helm chart now can ship with, is outdated. This is an off topic discussion though.

On topic though, perhas the fix would be to make the pre-commit hook about frigate use a conda environment instead. Then, you can specify helm and get that installed. If you have helm installed via conda, maybe we can manage?

Looking at the firgate code you linked, I conclude that frigate does indeed assume helm to be around but will do the helm dep update part. I see two options.

1. Frigate is updated to not care about dependency chart's configuration, but instead only documents the main chart's configuration.
2. The pre-commit hook referencing frigate specifies a conda environment instead of a pip environment to help us get helm installed.

[jacobtomlinson]

Overall, I think the practice of generating these kind of reports in the markdown document alongside values.yaml and alongside a values.schema.json file that helm chart now can ship with, is outdated. This is an off topic discussion though.

I would be keen to dig into this comment elsewhere. Frigate supports both generating static documentation files and also has a Sphinx extension. I'm curious what the replacement is for this kind of documentation? I feel I'm a little out of touch with being a helm chart user, so if folks are going elsewhere for configuration docs I'd be keen to learn.

On topic though, perhas the fix would be to make the pre-commit hook about frigate use a conda environment instead. Then, you can specify helm and get that installed. If you have helm installed via conda, maybe we can manage?

Ah I see it's the helm dependency that pre-commit-ci can't handle! I wasn't aware of the conda support in pre-commit that's awesome. That definitely seems like a good way forwards.

[consideRatio]

@jacobtomlinson I've opened rapidsai/frigate#44 which I think also fixes a few bugs that we have managed to survive because the following parts, which shouldn't be needed as that is supposed to be done by frigate.

helm-chart/.github/workflows/pre-commit.yml

Lines 15 to 17 in b09f9b2

	- run: |
	helm dependencies update ./dask
	helm dependencies update ./daskhub

[consideRatio]

@jacobtomlinson I updated this PR to also remove the helmlint hook. I don't want to go for an upstream contribution to change their pre-commit hook to be a conda-environment that provides helm, and it is also a hook that requires helm to be part of the system.

By removing this, we don't reduce our test suites capability to catch things. What helm lint does in its non --strict fashion is already tested by the current workflows that runs helm template and helm template --validate etc.

Action points

• Resolve Bugfix for charts with dependencies and make pre-commit hook function on pre-commit.ci rapidsai/frigate#46 in some way so that we have fixed a relevant bug and made the pre-commit hook function with pre-commit.ci
• Let Update daskhub's pangeo/base-notebook version to 2022.05.18 #274 be re-created by closing it and running the watch-dependencies workflow via the button available in https://github.com/dask/helm-chart/actions/workflows/watch-dependencies.yml, and verify that pre-commit.ci pushes a commit to the PR.
• Cut a release of frigate so that we can reference it from .pre-commit-config.yaml, with the new changes.
• Merge this PR
• Activate pre-commit.ci as a service for this project.
  This is done by first visiting the pre-commit.ci GitHub app and making it available for the dask GitHub organization to use: https://results.pre-commit.ci/. Then by declaring it to be used specifically for this github repository at: https://github.com/organizations/dask/settings/installations. Note that I don't have rights to do this and require this to be done by a dask org admin.

[consideRatio]

@jacobtomlinson we can start using pre-commit.ci and automatically provided formatting soon. We need a new frigate release (or reference the current default branch in frigare, and we need to activate pre-commit.ci for this project.

[jacobtomlinson]

Thanks @consideRatio. I've pinged RAPIDS Ops to get a release of frigate out. I'll let you know when it is done.

[consideRatio]

Thanks @jacobtomlinson!!

[consideRatio]

@jacobtomlinson at this point with the 0.6.0 release out (rapidsai/frigate#55), I think things will work with pre-commit.ci, and we will get automatic frigate commits added to PRs needing them!

Action points

• Review/Merge PR
• Activate pre-commit.ci for this project at https://github.com/organizations/dask/settings/installations

[jacobtomlinson]

🎉

[consideRatio]

Wieee @jacobtomlinson!!!