Skip to content
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

feat: add precommit #7056

Closed
wants to merge 20 commits into from
Closed

feat: add precommit #7056

wants to merge 20 commits into from

Conversation

juju4
Copy link
Contributor

@juju4 juju4 commented Jan 8, 2023

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
master...juju4:Azure-Sentinel:devel-precommit2

Follow-up hooks here or new PR:

@juju4 juju4 requested review from a team as code owners January 8, 2023 16:11
@v-atulyadav v-atulyadav self-assigned this Jan 9, 2023
@v-atulyadav v-atulyadav added the enhancement New feature or request label Jan 10, 2023
@v-amolpatil
Copy link
Contributor

@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.

@v-amolpatil
Copy link
Contributor

@juju4 Could you please take a look at previous comment?

@juju4
Copy link
Contributor Author

juju4 commented Jan 14, 2023

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.

@v-amolpatil
Copy link
Contributor

We will take a look. Thanks!

@v-amolpatil v-amolpatil assigned rushriva and unassigned v-amolpatil Jan 23, 2023
@juju4 juju4 requested review from a team as code owners February 4, 2023 20:36
@juju4
Copy link
Contributor Author

juju4 commented Feb 4, 2023

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

@juju4
Copy link
Contributor Author

juju4 commented Feb 11, 2023

Any input here?

Should I revert remediation and put test in continue-on-error like #7323 ?

@juju4
Copy link
Contributor Author

juju4 commented Feb 25, 2023

News ?

@v-atulyadav
Copy link
Contributor

Hi @mkchiliveri, please provide your response on this. Thanks

@v-atulyadav
Copy link
Contributor

Hi @daxesht, could you please provide your feedback on this. Thanks

@v-atulyadav
Copy link
Contributor

Hi @daxesht, please provide your feedback on this. Thanks

@v-atulyadav
Copy link
Contributor

Hi @daxesht, could you please provide your feedback on this. Thanks

@v-atulyadav
Copy link
Contributor

Hi @daxesht, please provide your feedback on this. Thanks

@juju4
Copy link
Contributor Author

juju4 commented Jun 17, 2023

Any update on this or path forward?

@v-atulyadav
Copy link
Contributor

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

@v-atulyadav
Copy link
Contributor

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

@v-atulyadav
Copy link
Contributor

Hi @juju4, waiting for your response over above comments and also please resolve branch conflicts. Thanks

@v-atulyadav
Copy link
Contributor

Hi @juju4, please check comments above and act accordingly. Thanks

@v-atulyadav
Copy link
Contributor

Hi @juju4, I am looking forward to your response to the above comments, as well as the resolution of branch conflicts.
Thanks

@v-atulyadav
Copy link
Contributor

Hi @juju4, please respond over above comments and also please resolve branch conflicts. Thanks

@v-atulyadav
Copy link
Contributor

Hi @juju4, please check comments above and act accordingly. Thanks

@v-atulyadav
Copy link
Contributor

Hi @juju4, hope you are doing well. Just wanted to check if you got a chance to look at the feedback shared. Please feel free to reach out to us for any queries and/or support. Thanks

@juju4
Copy link
Contributor Author

juju4 commented Aug 5, 2023

The content of the PR has been explained in #7056 (comment)
It applies to both Github Action and ADO because ultimately it should be available to whatever backend is used as pipeline with Sentinel Repository feature.

It adds no functionality outside of validating more content but goes further than existing validations. same than #7323

@v-atulyadav
Copy link
Contributor

Noted @juju4,
We will check internally and provide you with an update as soon as possible.
Thanks

@v-atulyadav
Copy link
Contributor

Hi @juju4,
We will provide you with an update on this PR by the middle of next week, and if we find any difficulty, we will reach out to you.
Thanks.

@mkchiliveri
Copy link
Contributor

Hi @juju4 ,

As discussed, can you please check on if we can enforce this rule only on files committed as part of PR so that author can take an action immediately instead of the full review.

@juju4
Copy link
Contributor Author

juju4 commented Sep 2, 2023

Per pre-commit-ci/issues#90, this is not possible. at least, not with the tool.

As said, the normal way would be cleaning existing repo progressively. Part work done in following commits that were reverted to keep this PR small. Can push them back after but for separate PR.
84ca86a
f24f041
8422184

@v-atulyadav
Copy link
Contributor

Hi @juju4, as discussed we will take this as internal feature and try to develop this in future. Thanks

@v-atulyadav v-atulyadav closed this Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants