-
Notifications
You must be signed in to change notification settings - Fork 0
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 the way API KEY is read #2
Conversation
ketakipai
commented
Nov 22, 2022
- taking path of env file from user and using it to load environment.
- getting api key from loaded environment.
@ketakipai I would like to flag this for dev discussion. |
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.
I suggest we do the simple changes only. I really think that we should try the pre-commit in our own development lifecycle sooner than later because this will likely uncover un-expected problems/limitations.
We can polish the project configuration once we have confidence that there are no major blockers when using the hook in a developer setup.
You can add the hook by following the README here https://github.com/ketaki99/test-pre-commit |
README.md
Outdated
pass_filenames: false | ||
additional_dependencies: ["requests"] | ||
verbose: true | ||
language: python |
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.
please remove these ones as well. As you can see here https://pre-commit.com/#pre-commit-configyaml---hooks only the verbose option works, the other options do not exist for .pre-commit-config.yaml
.
setup.py
Outdated
@@ -10,7 +10,11 @@ | |||
author="Private AI", | |||
description=DESCRIPTION, | |||
packages=find_packages(), | |||
install_requires=["pre-commit==2.20.0", "requests==2.28.1"], | |||
install_requires=[ | |||
"pre-commit==2.20.0", |
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.
We shouldn't install pre-commit as a dependency. pre-commit is the tool that installs our tool so we are not dependent on it.
…that we need the rest
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.
@ketakipai can you please update the README to only include the valid .pre-commit-config.yaml
fields? Currently, you only removed one of the invalid ones but there are still invalid ones on there which can cause confusion and unknown issues later on. I have already attached the documentation but I am attaching it again: https://pre-commit.com/#pre-commit-configyaml---hooks
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.
I have pointed all the .pre-commit-config.yaml
options that we need to change in our documentation in order to provide the correct information to our customers.
README.md
Outdated
repos: | ||
- repo: https://github.com/ketaki99/test-pre-commit | ||
rev: test.1 | ||
rev: test.5 | ||
hooks: | ||
- id: pii-check | ||
name: Check for PII |
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 option is used to override the name of the hook and it is optional. I would rather not include this in our documentation since we are already setting this name in .pre-commit-hooks.yaml
.
README.md
Outdated
repos: | ||
- repo: https://github.com/ketaki99/test-pre-commit | ||
rev: test.1 | ||
rev: test.5 | ||
hooks: | ||
- id: pii-check | ||
name: Check for PII | ||
description: this hook checks if staged files have PII and marks it. |
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.
Not a valid .pre-commit-config.yaml
keyword.
README.md
Outdated
hooks: | ||
- id: pii-check | ||
name: Check for PII | ||
description: this hook checks if staged files have PII and marks it. | ||
entry: pii_check --url URL --enabled-entities ENABLED_ENTITIES | ||
entry: pii_check --url URL --env-file-path ENV_FILE_PATH --enabled-entities ENABLED_ENTITIES |
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.
Not a valid .pre-commit-config.yaml
keyword. We should instead use args
keyword. This means that we need to update our entry
keyword in .pre-commit-hooks.yaml
.
README.md
Outdated
hooks: | ||
- id: pii-check | ||
name: Check for PII | ||
description: this hook checks if staged files have PII and marks it. | ||
entry: pii_check --url URL --enabled-entities ENABLED_ENTITIES | ||
entry: pii_check --url URL --env-file-path ENV_FILE_PATH --enabled-entities ENABLED_ENTITIES | ||
pass_filenames: false |
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.
Not a valid .pre-commit-config.yaml
keyword. Please remove.
.pre-commit-hooks.yaml
Outdated
name: Check for PII | ||
description: this hook checks if modified files have PII and marks it. | ||
entry: pii_check --url URL --enabled-entities ENABLED_ENTITIES | ||
entry: pii_check --url URL --env-file-path ENV_FILE_PATH --enabled-entities ENABLED_ENTITIES |
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.
Due to changes we are going to do, this entry
should be updated to be only the following:
entry: pii_check --url URL --env-file-path ENV_FILE_PATH --enabled-entities ENABLED_ENTITIES | |
entry: pii_check |
I'll wait for @guyd and @onurcayci to approve before taking a look |
@ketakipai could you also please merge master into this branch and fix the conflict please? |
.pre-commit-hooks.yaml
Outdated
args: | ||
[ | ||
--url=URL, | ||
--env-file-path=ENV_FILE_PATH, | ||
--enabled-entities=ENABLED_ENTITIES, | ||
] |
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 looks incorrect, I don't think this is the valid way of passing arguments.
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.
Here is an example: https://github.com/python-poetry/poetry/blob/master/.pre-commit-hooks.yaml
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.
Since we are passing enabled_entities as a list of args, the traditional way does not work. I referred to pre-commit/pre-commit#971 and changed the other 2 too for uniformity
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.
Can I ask why we need these args here? Looking at the link you shared, the args we have defined here are always passed to the entry point. We are also defining these args in the .pre-commit-config.yaml
, which looks like it can cause runtime issues. If this is only to document the available flags that we can pass to the hook, I would find a different way to document them and remove this.
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.
Yes this is mainly a placeholder for args, I have tested with this in pre-commit-hooks and found no issues. The script takes care of optional and non-optional args and doesn't throw an error if enabled-entities is not passed.
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.
If they are placeholder, then we should remove them. Documentation is where we should be showing these things.
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.
I tested that the hook works without these, removing them
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.
Did a quick review. Looks good.
.pre-commit-hooks.yaml
Outdated
args: | ||
[ | ||
--url=URL, | ||
--env-file-path=ENV_FILE_PATH, | ||
--enabled-entities=ENABLED_ENTITIES, | ||
] |
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.
Can I ask why we need these args here? Looking at the link you shared, the args we have defined here are always passed to the entry point. We are also defining these args in the .pre-commit-config.yaml
, which looks like it can cause runtime issues. If this is only to document the available flags that we can pass to the hook, I would find a different way to document them and remove this.
.pre-commit-hooks.yaml
Outdated
args: | ||
[ | ||
--url=URL, | ||
--env-file-path=ENV_FILE_PATH, | ||
--enabled-entities=ENABLED_ENTITIES, | ||
] |
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.
args: | |
[ | |
--url=URL, | |
--env-file-path=ENV_FILE_PATH, | |
--enabled-entities=ENABLED_ENTITIES, | |
] |
README.md
Outdated
entry: pii_check --url URL --enabled-entities ENABLED_ENTITIES | ||
pass_filenames: false | ||
additional_dependencies: ["requests"] | ||
entry: pii_check |
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.
One last thing, this is not a valid .pre-commit-config.yaml
field. Please remove it.
entry: pii_check |
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.
Done!
* added all files * updated README * Deid 1284 improve robustness of pii check on off flags (#3) * fixed bugs with the PII_CHECK flags * removed local hook * added suggestion by Guy * added Guys suggestion and removed local hook * DEID-1311-update-git-pre-commit-hook-documentation (#4) * updated README * did the suggested changes * Update the way API KEY is read (#2) * passing path of env file as variable * remove the unnecessary line from setup.py * bug fix and updated script to get api key from env file * updated README * removed pathlib2 dependency and added stuff to check for relative path * updated README * added validation to check if API_KEY is missing * updated README * added all dependencies in setup.py, removed from config * removing unnecessary dependecies from pre-commit-hooks * updated dependencies * removed unnecessary fields mentioned in README, tested and confirmed that we need the rest * updated README * updated README * updated README * updated pre-commit-hooks * removing args from pre-commit-hooks * removing entry from pre-commit-config * updated README to resolve conflicts * Make compatible with beta2 (#5) * test * updated fields to be compatible with beta 2 * removing line added for debugging * updated README to contain latest rev * update README to have new repo * added link_batch and removed an unused function (#6) * added link_batch and removed an unused function * removed test file