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

[Lint] Add bat version and lint CI permission #132

Merged
merged 6 commits into from
Sep 28, 2022
Merged

[Lint] Add bat version and lint CI permission #132

merged 6 commits into from
Sep 28, 2022

Conversation

titaiwangms
Copy link
Contributor

@titaiwangms titaiwangms commented Sep 26, 2022

  1. Add bat version linter for Windows user
  2. Add permission setting for third-party actions in lint.yaml to limit their access
  3. Did some research and discussed with @justinchuby, CodeQL only hints on the modified file in a PR, and the error/warning/note are all informative, and aligned with pylint/mypy which are included in out lint, so I suggest we can have it for a while, and see how it goes. (However, disable one or few rules that we don't need in CodeQL seems doable in the next release: How to disable particular rule by its ID from GitHub workflow? github/codeql#7937)

fixes #127

@@ -10,6 +10,19 @@ jobs:
lint-python:
name: Lint Python
runs-on: ubuntu-latest
permissions:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I referenced from Pytorch/TensorRT setting, which is also using third-party actions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think only lint-python will need to set permissions because others don't use 3p actions.

Comment on lines 51 to 63
permissions:
actions: write
checks: write
contents: write
deployments: none
id-token: write
issues: write
discussions: write
packages: write
pull-requests: write
repository-projects: none
security-events: none
statuses: write
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
permissions:
actions: write
checks: write
contents: write
deployments: none
id-token: write
issues: write
discussions: write
packages: write
pull-requests: write
repository-projects: none
security-events: none
statuses: write

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to change those not using third-party as well?

Copy link
Collaborator

@justinchuby justinchuby Sep 27, 2022

Choose a reason for hiding this comment

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

these jobs are fine. as long as they are not 3p github actions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then this one if good to go?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just remove the whole permissions section for these two jobs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That won't work. It seems if you have one with specify permissions, you will have to define others as well, based on my trial.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm that's good to know. kinda surprising that it works this way though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, I just checked my commit. I think I haven't tried that. Let me give it a try.

id-token: write
issues: none
discussions: none
packages: none
Copy link
Collaborator

Choose a reason for hiding this comment

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

none fields can be removed because "If you specify the access for any of these scopes, all of those that are not specified are set to none." https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think one of the benefits of leaving it there is that the next person, who needs to change the permission, can see what options he/she has on this (No need to investigate again). Besides, we are not adding more lines on this. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds good to me

@titaiwangms
Copy link
Contributor Author

cc @gramalingam for batch files review.

Copy link
Collaborator

@gramalingam gramalingam left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@titaiwangms titaiwangms merged commit 05778ce into microsoft:main Sep 28, 2022
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.

[Lint] Wrap-up lint adding
3 participants