Azure
feat: add precommit
Change(s):
- add pre-commit client config and github workflow+azure-pipelines to run server side
Reason for Change(s):
- add more early controls in "python standard" way
Version Updated:
- No
Testing Completed:
- Yes (github workflow only and private azure-pipelines)
Checked that the validations are passing and have addressed any issues that are present:
- No change to code. Add validation check only.
This PR adds precommit config
Example run as is without fixing anything https://github.com/juju4/Azure-Sentinel/actions/runs/3867376910/jobs/6592111638#step:4:52
Following fixed initial config, not sure if you prefer to be in this PR or a different one https://github.com/juju4/Azure-Sentinel/actions/runs/3867831878 https://github.com/Azure/Azure-Sentinel/compare/master...juju4:Azure-Sentinel:devel-precommit2
Follow-up hooks here or new PR:
- codespell - https://github.com/codespell-project/codespell https://github.com/juju4/Azure-Sentinel/commit/b079d3657003453bfcb249c3f9ee9e1773fa89e9 a bit of remediation (there is a lot here...) https://github.com/juju4/Azure-Sentinel/actions/runs/3867789949/jobs/6592801053
- detect-secrets - https://github.com/Yelp/detect-secrets
- PSScriptAnalyzer - https://github.com/PowerShell/PSScriptAnalyzer
@juju4 Could you please resolve the conflicts? Also could you please fix the build errors. Can you please provide more details on the pre-commit? Is it for validating the spelling mistakes in any of the file.
I merged latest upstream to address conflict. As for precommit pipelines, please confirm that I should merge remediation branches too.
On codespell remediations, only doing part of them as too many. Each content owner should address their own. To avoid pipeline failing, I'm putting the non-remediated folders in exclude list
arm-ttk pipeline failing part is new and should not be related to PR as no functional change.
I applied remediation changes to branch to fix precommitValidation pipeline (but impacting tons of files as json prettify/standardize, end of files, UTF-8 BOM, duplicate key and so on) Not sure about the failing check if related or not as can't see details for ADO tests.
codespell and the rest in future PR
Any input here?
Should I revert remediation and put test in continue-on-error like https://github.com/Azure/Azure-Sentinel/pull/7323 ?
@juju4 Why are we required to remove spaces in all the files in the repo? Is this really required? @v-atulyadav Can you communicate the requirement of this change offline? I don't see any code changes in this PR. And in fact it is very difficult to even judge the required changes in vast set of 5500+ files. Please work with the author to remove the files which are not required for this PR before any evaluation is initiated.
I reverted the content changes, merged current master and added continue-to-error to not failing pipelines, but the goal of pre-commit is early validation and as such, it
- checks validity of file, partly similar to existing controls
- prettify/standardize formatting of some files like json. see
- id: pretty-format-json
args:
- "--autofix"
- "--indent=4"
- "--no-sort-keys"
Indent with 4 spaces is what I observed the most, typically from Azure portal export template.
But else, current repository content has various format/indentation. There are also content with utf8 bom, duplicate key and other issues. This may be without impact when used in Sentinel initially, now but may not always be the case.
With the continue-on-error, change should be painless but content owners need to review so at some point, it is not just pipeline warnings. Applied successfully in my production environment but subset of this repo.
Hi @devikamehra, @juju4 has responded on your comments please check for the same. Thanks
@juju4 Thank you so much for clarifying. @v-atulyadav Can you work with Prateek on this? Should we be making this change, what will be the impact, etc.
Hi @juju4, could you please give us a brief description of this PR. Thanks
Hi @devikamehra, mahesh will look into this. Thanks
Hi @mkchiliveri, can you please provide your response on this. Thanks
Description of PR is in above comments but let's try to reword
- this add a .pre-commit-config file
- main goal is to use on developer client aka pre-commit
- still also enabled "server" side aka ADO+Github as sometimes client side not done
The pre-commit validation includes default action from https://pre-commit.com/
- ensure yaml and json file are valid
- remove extra end-of-file
- check for case conflict
- check for private key
- check for requirements.txt (python)
- ensure not adding large file by mistakes (if limit too low, should be raised. likely with solutions zip inclusion)
- prettify/normalize json file in order to make them easier to compare
More can be added later like codespell, powershell lint, other secrets check and so on.
Hi @mkchiliveri, could you please provide your response on this. Thanks
Hi @mkchiliveri, could you please provide your response on this. Thanks
Hi @mkchiliveri, could you please provide your response on this. Thanks
Hi @mkchiliveri, could you please provide your response on this. Thanks
Hi @juju4, I apologize for the inconvenience. We will provide you with an update by the 13th. Please update your branch from master in the meantime. Thanks
Hi @juju4, Having analyzed the contents, we are not able to determine the reason for this change. Could you please elaborate on this and provide screenshots of this functionality if possible? Also the changes you are requesting are for both Github workflows and Azure pipelines, can't we handle this in one place instead of both? It would also be great if we could connect over a call to discuss further. Thanks
Hi @juju4, waiting for your response over above comments and also please resolve branch conflicts. Thanks