Skip to content

Conversation

@krithika369
Copy link
Collaborator

@krithika369 krithika369 commented Aug 7, 2022

The motivation for this PR is to update the Helm template value in infra/charts/turing/templates/_helpers.tpl for the postgres port used, which was missed from the PR that updated it earlier: #208

The incorrect template expression did not cause any problems in the e2e tests because the port used was 5432, which is the same as the default port. Nonetheless, this PR corrects the mistake.

In addition, the pre-commit tool has been integrated which helps automate some additional checks / actions when some files are being modified (eg: regenerating the Helm docs with changes to the chart values). The .pre-commit-config.yaml file has each of these hooks:

  • Helm docs generation
  • OpenAPI Python client generation (local hook, using the respective make target)
  • Linting of Golang, Js, Python code (local hook, using the respective make target)

Notes

  • README.md - contains the installation details for the pre-commit hooks. We must run make setup on the repo once. Prior to this, it may be required to clear existing hooks (such as from husky, described further below) from the local environment, if the repo has already been cloned and used in the past.
$ git config --unset-all core.hooksPath
$ rm -rf .git/hooks
$ make setup
  • Makefile - adding the dev setup target. Corresponding setup targets from other Makefiles are now redundant and have been removed.
  • ui/package.json - Removing husky and lint-staged scripts and dev dependencies - these will now be covered by the common pre-commit hook. Having husky manage the hooks also conflicts with the pre-commit tool as they will both end up having to use the same git config value core.hooksPath.
  • One notable difference between the husky hook for prettifying the UI vs the pre-commit hook is that any automatically modified files will not be automatically staged and added to the commit. Rather, the commit action will fail with a message and the developer is expected to git add the auto-modified files too. This behavior is the same for all pre-commit hooks and is apparently a conscious design decision (see functionality to add files to commit pre-commit/pre-commit#1099).

Screenshot 2022-08-07 at 3 27 02 PM

  • Using the files filter in the pre-commit hook definitions can limit the running of hooks based on the modified files (instead of running all hooks always).

Screenshot 2022-08-07 at 11 27 42 PM

Most other modifications in this PR are auto-formatting fixes, particularly in the Python projects, from integrating black.

@krithika369 krithika369 marked this pull request as draft August 7, 2022 15:29
@krithika369 krithika369 changed the title Draft: Fix helm chart template value and add pre-commit hooks Fix helm chart template value and add pre-commit hooks Aug 8, 2022
@krithika369 krithika369 marked this pull request as ready for review August 8, 2022 01:19
@krithika369 krithika369 requested a review from a team August 8, 2022 01:42
Copy link
Collaborator

@romanwozniak romanwozniak left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -0,0 +1,63 @@
import time
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this file committed unintentionally?

@krithika369 krithika369 closed this Aug 8, 2022
@krithika369 krithika369 force-pushed the helm_chart_fixes_precommit branch from 4fbab6f to cf7408d Compare August 8, 2022 07:19
@krithika369 krithika369 reopened this Aug 8, 2022
@krithika369 krithika369 merged commit 20989ac into caraml-dev:main Aug 15, 2022
@krithika369 krithika369 deleted the helm_chart_fixes_precommit branch August 15, 2022 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants