Initial pre-commit linting #1539

shaidar · 2024-04-26T16:00:21Z

What's this PR do?

This is initial and incomplete work on updating the pre-commit hooks. I was unable to get the prettier plugin to run locally due to package.json dependency issues and think someone more familiar with the project and those packages should take a closer look at resolving those.

Anas12091101 · 2024-05-07T10:28:32Z

I'll be taking a look at the failed checks for this PR

Anas12091101 · 2024-05-09T09:50:53Z

@shaidar the checks are now passing. Do you know why there's no pre-commit check here?

arslanashraf7 · 2024-05-14T11:54:15Z

.github/workflows/ci.yml

+      # TODO: Update Pylint or Integrate ruff
+      #      - name: Lint
+      #        run: poetry run pylint ./**/*.py


I think we should remove this since pre-commit should handle this.

arslanashraf7 · 2024-05-14T14:12:03Z

.pre-commit-config.yaml

+          - --exclude-files
+          - ".*_test.js"
+          - --exclude-files
+          - "static/js/constants.js"


I don't think we should ignore this file. This file is prone to having secrets

arslanashraf7 · 2024-05-14T14:13:28Z

.prettierignore

+static/js/components/NewApplication.js
+static/js/components/forms/ProfileFormFields.js
+static/js/components/forms/elements/FormError.js
+main/templates/background-images.css


This can be ignored, But I'm curious why we want to ignore prettier on this?

Perhaps, @shaidar could provide a better answer to this

arslanashraf7 · 2024-05-14T14:49:03Z

.secrets.baseline

    },
    {
-      "name": "ArtifactoryDetector"
+      "name": "AWSKeyDetector"
    },
    {


I was comparing it with xPRO where we have also added "name": "AzureStorageKeyDetector"

arslanashraf7 · 2024-05-14T15:12:07Z

applications/apps.py

@@ -10,4 +10,3 @@ class ApplicationConfig(AppConfig):

    def ready(self):
        """Application is ready"""
-        import applications.signals  # pylint:disable=unused-import, unused-variable


This is needed to load the signals. We should ignore the error on this line instead. e.g. # noqa: F401

arslanashraf7 · 2024-05-15T09:59:05Z

scripts/test/detect_missing_migrations.sh

Why are we removing this file? If we've moved this step to somewhere else? Could you point me to that location?

@shaidar could tell better, the file was removed in one of his commit

These scripts checks if someone mistakenly forgot to create a migration file against the changes made in the Django models. It's important that we check this.

Feel free to add them back

arslanashraf7 · 2024-05-15T10:00:16Z

scripts/test/no_auto_migrations.sh

Same as above comment. Why this file was removed

arslanashraf7 · 2024-05-15T10:01:53Z

scripts/test/python_tests.sh

 }

 run_test black --check .
 run_test pytest --no-pylint
-run_test ./scripts/test/detect_missing_migrations.sh


If we put the missing migrations scripts back, we will need to update this as well.

arslanashraf7 · 2024-05-15T15:03:29Z

static/js/hello_test.js

I think we should remove this file.

arslanashraf7 · 2024-05-15T15:42:14Z

docker-compose.travis.yml

We don't use travis now, I think we should remove this file.

gitguardian · 2024-05-21T07:03:04Z

⚠️ GitGuardian has uncovered 10 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
9430286	Triggered	Generic Password	`8d4b9e3`	.github/workflows/ci.yml	View secret
9430286	Triggered	Generic Password	`8d4b9e3`	.github/workflows/ci.yml	View secret
9430286	Triggered	Generic Password	`e1acbe9`	.github/workflows/ci.yml	View secret
9430286	Triggered	Generic Password	`e1acbe9`	.github/workflows/ci.yml	View secret
9430286	Triggered	Generic Password	`0f7f2b9`	.github/workflows/ci.yml	View secret
9430286	Triggered	Generic Password	`29a9486`	.github/workflows/ci.yml	View secret
9430286	Triggered	Generic Password	`0f7f2b9`	.github/workflows/ci.yml	View secret
9430286	Triggered	Generic Password	`3a90bda`	.github/workflows/ci.yml	View secret
9430286	Triggered	Generic Password	`3a90bda`	.github/workflows/ci.yml	View secret
9430286	Triggered	Generic Password	`29a9486`	.github/workflows/ci.yml	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

arslanashraf7

Thanks for making all the changes. I smoke tested the application overall and things seem to be working fine.

I just left one last comment, Fee free to merge after that. Don't forget to squash merge.

arslanashraf7 · 2024-05-27T11:49:49Z

main/settings.py

+# fmt: off
+PASSWORD_RESET_CONFIRM_URL = "password_reset/confirm/{uid}/{token}/"  # pragma: allowlist secret
+# fmt: on


Can you cross-match the same setting with xPRO, We didn't to turn the fmt off/on there. Instead, we used

PASSWORD_RESET_CONFIRM_URL = "password_reset/confirm/{uid}/{token}/" # noqa: S105 # pragma: allowlist secret

* Initial pre-commit linting * fix: worked on the suggestions * fix: fixed some styling issues * fix: fixed some styling issues * fix: ignoring git guardian on a line in ci.yml * revert: ignoring git guardian on a line in ci.yml * fix: replaced fmt:off with noqa * fix: black formatting issue --------- Co-authored-by: Muhammad Anas <[email protected]>

* fix(deps): update dependency ramda to ^0.30.0 * fix: manual ramda fixes * Initial pre-commit linting (#1539) * Initial pre-commit linting * fix: worked on the suggestions * fix: fixed some styling issues * fix: fixed some styling issues * fix: ignoring git guardian on a line in ci.yml * revert: ignoring git guardian on a line in ci.yml * fix: replaced fmt:off with noqa * fix: black formatting issue --------- Co-authored-by: Muhammad Anas <[email protected]> * Initial pre-commit linting (#1539) * Initial pre-commit linting * fix: worked on the suggestions * fix: fixed some styling issues * fix: fixed some styling issues * fix: ignoring git guardian on a line in ci.yml * revert: ignoring git guardian on a line in ci.yml * fix: replaced fmt:off with noqa * fix: black formatting issue --------- Co-authored-by: Muhammad Anas <[email protected]> * fix: tests * fix: formatting issues * fix: some cleanup --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Asad Ali <[email protected]> Co-authored-by: Sar <[email protected]> Co-authored-by: Muhammad Anas <[email protected]>

shaidar added help wanted Needs Review labels Apr 26, 2024

Anas12091101 self-assigned this May 7, 2024

Anas12091101 requested a review from asadali145 May 9, 2024 13:40

arslanashraf7 self-requested a review May 13, 2024 13:13

arslanashraf7 self-assigned this May 13, 2024

arslanashraf7 requested changes May 16, 2024

View reviewed changes

arslanashraf7 added Waiting on Author and removed help wanted Needs Review labels May 16, 2024

Anas12091101 force-pushed the sar/pre_commit_linting branch from 00d1f02 to 4d29212 Compare May 21, 2024 07:02

Anas12091101 requested a review from arslanashraf7 May 21, 2024 10:06

Anas12091101 added Needs Review and removed Waiting on Author labels May 21, 2024

shaidar and others added 6 commits May 27, 2024 06:32

Initial pre-commit linting

3a90bda

fix: worked on the suggestions

a981e7f

fix: fixed some styling issues

e1acbe9

fix: fixed some styling issues

0f7f2b9

fix: ignoring git guardian on a line in ci.yml

29a9486

revert: ignoring git guardian on a line in ci.yml

8d4b9e3

Anas12091101 force-pushed the sar/pre_commit_linting branch from a2f6573 to 8d4b9e3 Compare May 27, 2024 06:46

arslanashraf7 approved these changes May 27, 2024

View reviewed changes

Anas12091101 added 2 commits May 27, 2024 12:35

fix: replaced fmt:off with noqa

c943e5b

fix: black formatting issue

9859e07

Anas12091101 merged commit 8e6c070 into master May 27, 2024
5 checks passed

Anas12091101 deleted the sar/pre_commit_linting branch May 27, 2024 13:00

odlbot mentioned this pull request May 29, 2024

Release 0.118.0 #1556

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial pre-commit linting #1539

Initial pre-commit linting #1539

shaidar commented Apr 26, 2024

Anas12091101 commented May 7, 2024

Anas12091101 commented May 9, 2024

arslanashraf7 May 14, 2024

arslanashraf7 May 14, 2024

arslanashraf7 May 14, 2024

Anas12091101 May 21, 2024

arslanashraf7 May 14, 2024

arslanashraf7 May 14, 2024

arslanashraf7 May 15, 2024

Anas12091101 May 21, 2024

arslanashraf7 May 21, 2024

shaidar May 21, 2024

arslanashraf7 May 15, 2024

shaidar May 21, 2024

arslanashraf7 May 15, 2024

arslanashraf7 May 15, 2024

arslanashraf7 May 15, 2024

gitguardian bot commented May 21, 2024 •

edited

Loading

arslanashraf7 left a comment

arslanashraf7 May 27, 2024

Initial pre-commit linting #1539

Initial pre-commit linting #1539

Conversation

shaidar commented Apr 26, 2024

What's this PR do?

Anas12091101 commented May 7, 2024

Anas12091101 commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitguardian bot commented May 21, 2024 • edited Loading

⚠️ GitGuardian has uncovered 10 secrets following the scan of your pull request.

arslanashraf7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitguardian bot commented May 21, 2024 •

edited

Loading