Add a soundness check to lint Python files #14

ahoppen · 2024-10-03T23:58:35Z

No description provided.

In preparation for swiftlang/github-workflows#14

.github/workflows/soundness.yml

shahmishal

LGTM!

shahmishal · 2024-10-10T08:08:46Z

@FranzBusch, please feel free to merge it once you’ve reviewed it.

FranzBusch · 2024-10-10T10:07:42Z

I just merged your other PR @ahoppen but now that I think about it again we might want to fold this into the Swift format PR after all. The reason why is that each new check here is producing a separate check on the repos. Now most repos don't have all the languages. It might be better to just have one general formatting job that does

Shellcheck
swift-format
yamlint
python lint
etc.
WDYT?

ahoppen · 2024-10-10T18:16:32Z

I don’t have a strong personal opinion here:

I think the advantages of having a single job are:

We only create a single item in the status check for a PR, which is less noisy

The disadvantages are:

Running swift-format requires us to use a Swift container, all other tools can run on the image itself. So, either we need to install yamllint etc. in the container or have one job for swift-format and one for all the things that can run on the runner itself.
If eg. yamllint fails, we probably won’t continue execution, which means that you might only find the shellcheck issues after fixing all YAML issues. Probably not a huge problem in practice though.
If you disable a check, we would just skip it in the job and wouldn’t mark it as skipped in the GitHub PR check UI. Not sure if that’s an advantage or disadvantage though.

@shahmishal What’s your opinion?

shahmishal · 2024-10-10T19:19:57Z

Having checks for each lang format/lint makes it easier to figure out what failed. Normally people will ignore all of the green check marks and focus on failures.

ahoppen · 2024-10-10T20:11:21Z

OK, sounds like we want to stick with separate jobs for each lint then.

FranzBusch

Okay let's proceed with this then. Just some naming nits to stay consistent

FranzBusch · 2024-10-11T10:59:36Z

.github/workflows/soundness.yml

@@ -59,6 +59,10 @@ on:
        type: boolean
        description: "Boolean to enable the YAML lint job. Defaults to true."
        default: true
+      python_lint_enabled:


Suggested change

python_lint_enabled:

python_lint_check_enabled:

FranzBusch · 2024-10-11T10:59:48Z

.github/workflows/soundness.yml

@@ -59,6 +59,10 @@ on:
        type: boolean
        description: "Boolean to enable the YAML lint job. Defaults to true."
        default: true
+      python_lint_enabled:
+        type: boolean
+        description: "Boolean to enable the Python lint job. Defaults to true."


Suggested change

description: "Boolean to enable the Python lint job. Defaults to true."

description: "Boolean to enable the Python lint check job. Defaults to true."

FranzBusch · 2024-10-11T11:00:00Z

.github/workflows/soundness.yml

@@ -200,3 +204,18 @@ jobs:
        run: |
          curl -s https://raw.githubusercontent.com/swiftlang/github-workflows/refs/heads/main/.github/workflows/configs/yamllint.yml > /tmp/yamllint.yml
          yamllint --strict --config-file /tmp/yamllint.yml ${GITHUB_WORKSPACE}
+  python-lint:


Suggested change

python-lint:

python-lint-check:

FranzBusch · 2024-10-11T11:00:06Z

.github/workflows/soundness.yml

@@ -200,3 +204,18 @@ jobs:
        run: |
          curl -s https://raw.githubusercontent.com/swiftlang/github-workflows/refs/heads/main/.github/workflows/configs/yamllint.yml > /tmp/yamllint.yml
          yamllint --strict --config-file /tmp/yamllint.yml ${GITHUB_WORKSPACE}
+  python-lint:
+    name: Python lint


Suggested change

name: Python lint

name: Python lint check

ahoppen · 2024-10-11T21:08:11Z

Should we go the opposite way and remove check from all of these jobs? It feels redundant to me because I would expect all CI jobs to check something.

ahoppen · 2024-10-11T22:04:18Z

Discussed offline that we want to use check in the name.

kkebo · 2024-10-11T23:22:29Z

.github/workflows/soundness.yml

+          persist-credentials: false
+      - name: Run flake8
+        run: |
+          pip3 install flake8 flake8-import-order --break-system-packages


As you may know, it seems that ubuntu-latest on GitHub-hosted runners is currently in the process of migrating to Ubuntu 24.04. Until it's completed, Ubuntu 22.04 may be used, and on Ubuntu 22.04, this command will fail because its pip3 is old and does not have --break-system-packages.

Announcement: actions/runner-images#10636

Failure case: https://github.com/kkebo/swift-format/actions/runs/11299915833/job/31431847497

Since the migration will be completed soon, I think we can ignore it.

Just fixed it: #28

ahoppen requested a review from shahmishal as a code owner October 3, 2024 23:58

ahoppen added a commit to ahoppen/swift-format that referenced this pull request Oct 4, 2024

Make build-script-helper.py pass flake8 linting

33a7020

In preparation for swiftlang/github-workflows#14

ahoppen added a commit to ahoppen/swift-format that referenced this pull request Oct 4, 2024

Make build-script-helper.py pass flake8 linting

65bac1e

In preparation for swiftlang/github-workflows#14

ahoppen mentioned this pull request Oct 4, 2024

Make build-script-helper.py pass flake8 linting swiftlang/swift-format#831

Merged

shahmishal reviewed Oct 4, 2024

View reviewed changes

.github/workflows/soundness.yml Outdated Show resolved Hide resolved

ahoppen force-pushed the python-lint branch from 18c2b95 to c81e8dc Compare October 4, 2024 16:28

FranzBusch requested changes Oct 9, 2024

View reviewed changes

.github/workflows/soundness.yml Outdated Show resolved Hide resolved

ahoppen force-pushed the python-lint branch 3 times, most recently from 6460120 to ac9d4de Compare October 9, 2024 16:09

ahoppen requested a review from FranzBusch October 10, 2024 00:22

shahmishal approved these changes Oct 10, 2024

View reviewed changes

FranzBusch approved these changes Oct 10, 2024

View reviewed changes

ahoppen force-pushed the python-lint branch from ac9d4de to eea5b80 Compare October 10, 2024 20:12

FranzBusch reviewed Oct 11, 2024

View reviewed changes

Add a soundness check to lint Python files

180fdb5

ahoppen force-pushed the python-lint branch from eea5b80 to 180fdb5 Compare October 11, 2024 22:02

ahoppen requested a review from a team as a code owner October 11, 2024 22:02

ahoppen merged commit 5411ae3 into swiftlang:main Oct 11, 2024
8 checks passed

ahoppen deleted the python-lint branch October 11, 2024 22:04

kkebo reviewed Oct 11, 2024

View reviewed changes

	description: "Boolean to enable the Python lint job. Defaults to true."
	description: "Boolean to enable the Python lint check job. Defaults to true."

Add a soundness check to lint Python files #14

Add a soundness check to lint Python files #14

Uh oh!

Conversation

ahoppen commented Oct 3, 2024

Uh oh!

Uh oh!

Uh oh!

shahmishal left a comment

Choose a reason for hiding this comment

Uh oh!

shahmishal commented Oct 10, 2024

Uh oh!

FranzBusch commented Oct 10, 2024

Uh oh!

ahoppen commented Oct 10, 2024

Uh oh!

shahmishal commented Oct 10, 2024

Uh oh!

ahoppen commented Oct 10, 2024

Uh oh!

FranzBusch left a comment

Choose a reason for hiding this comment

Uh oh!

FranzBusch Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

FranzBusch Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

FranzBusch Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

FranzBusch Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Oct 11, 2024

Uh oh!

ahoppen commented Oct 11, 2024

Uh oh!

Uh oh!

kkebo Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

ahoppen Oct 12, 2024

Choose a reason for hiding this comment

Uh oh!

kkebo Oct 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!