Skip to content

Conversation

@dixonjoel
Copy link
Collaborator

@dixonjoel dixonjoel commented Sep 22, 2025

What does this Pull Request accomplish?

Uses the newly created analyze-project actions from ni/python-actions repo.

Why should this Pull Request be merged?

Single sources many of the static analysis checks we were duplicating in many repos.

What testing has been done?

The PR checks on this PR.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 22, 2025

Test Results

   10 files  ±0     10 suites  ±0   25s ⏱️ -1s
  256 tests ±0    256 ✅ ±0  0 💤 ±0  0 ❌ ±0 
2 510 runs  ±0  2 510 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit f976c0a. ± Comparison against base commit 2e45f1b.

♻️ This comment has been updated with latest results.

@dixonjoel dixonjoel marked this pull request as ready for review September 24, 2025 20:56
@dixonjoel dixonjoel enabled auto-merge (squash) September 24, 2025 21:29
@dixonjoel
Copy link
Collaborator Author

@mikeprosserni @csjall This PR creates some new checks so I think we need to modify the required checks to exclude the existing 'Run CI / Check nipanel / Check nipanel' check. And add in the new ones as required if possible now, if not, after submission. E.g. 'Run CI / Check nipanel / Check nipanel (macos-latest, 3.9)'

@bkeryan
Copy link
Collaborator

bkeryan commented Sep 25, 2025

@mikeprosserni @csjall This PR creates some new checks so I think we need to modify the required checks to exclude the existing 'Run CI / Check nipanel / Check nipanel' check. And add in the new ones as required if possible now, if not, after submission. E.g. 'Run CI / Check nipanel / Check nipanel (macos-latest, 3.9)'

Yes, this is a downside of matrixing the checks.

@bkeryan
Copy link
Collaborator

bkeryan commented Sep 25, 2025

@mikeprosserni @csjall This PR creates some new checks so I think we need to modify the required checks to exclude the existing 'Run CI / Check nipanel / Check nipanel' check. And add in the new ones as required if possible now, if not, after submission. E.g. 'Run CI / Check nipanel / Check nipanel (macos-latest, 3.9)'

Yes, this is a downside of matrixing the checks.

https://github.com/ni/nipanel-python/blob/main/.github/workflows/CI.yml has a dependency chain report_test_results -> run_unit_tests -> check_nipanel.

Perhaps you could remove check_nipanel from the list of required checks and rely on "Test Results"? However, report_test_results has if: always(), and I think it may create a successful "Test Results" if zero tests run. Perhaps if: always() && jobs.check_nipanel.result == 'success' could work?

@bkeryan
Copy link
Collaborator

bkeryan commented Sep 25, 2025

@mikeprosserni @csjall @dixonjoel @mshafer-NI One more thing about the required checks: listing required checks for specific Python versions causes additional friction when changing the supported Python versions. If you change the required checks to use the new versions, what about release branches that use the old versions? Previously in measurement-plugin-python, nidaqmx-python, etc. we used legacy branch protection rules and ended up with a ton of duplicated branch protection rules for different release branches, which mostly differed by required checks (for unit tests).

I liked having a single check_panel required check, and I think matrixing it on OS makes sense because it solves the problem of installing platform-specific packages like pywin32, but I'm not sold on matrixing check_panel on Python version. Besides version-specific dependencies, what do we get out of this? Are there version-specific flake8 and mypy checks?

(BTW, if GitHub made it easier to aggregate job status correctly, I probably wouldn't be complaining about this.)

@mshafer-NI
Copy link
Collaborator

@mikeprosserni @csjall @dixonjoel @mshafer-NI One more thing about the required checks: listing required checks for specific Python versions causes additional friction when changing the supported Python versions. If you change the required checks to use ... Besides version-specific dependencies, what do we get out of this? Are there version-specific flake8 and mypy checks?

Flake8 -> mostly not. But mypy (using either the parameter for Python version, or different python versions to run it with) has shown to do well at flagging stdlib changes that cause potential issues (parents=True being valid in later Python version but used in a module that claims support for older one, or removal of stdlib api's, or changes where they can now return None).

@bkeryan
Copy link
Collaborator

bkeryan commented Sep 25, 2025

@mikeprosserni @csjall @dixonjoel @mshafer-NI One more thing about the required checks: listing required checks for specific Python versions causes additional friction when changing the supported Python versions. If you change the required checks to use ... Besides version-specific dependencies, what do we get out of this? Are there version-specific flake8 and mypy checks?

Flake8 -> mostly not. But mypy (using either the parameter for Python version, or different python versions to run it with) has shown to do well at flagging stdlib changes that cause potential issues (parents=True being valid in later Python version but used in a module that claims support for older one, or removal of stdlib api's, or changes where they can now return None).

Ok. I think we should change CI.yml to summarize the matrix checks in a single "Checks succeeded" job so we don't have to list OSes and Python versions in the branch protection ruleset.

@dixonjoel
Copy link
Collaborator Author

@bkeryan Updated to have a 'checks_succeeded' step. So once this goes in, we can update the required checks to simply that.

Also, the current state of the PR is depending on a commit of python-actions that is in my dev branch over there. Do we need to wait for that to be release? Or is it ok to let renovate update it with the version # next week?

@dixonjoel dixonjoel requested a review from bkeryan October 1, 2025 15:11
@dixonjoel dixonjoel force-pushed the users/jdixon/reuse-analyze-project-action branch from 79252d8 to 0956012 Compare October 7, 2025 15:12
@dixonjoel dixonjoel merged commit e4a7f97 into main Oct 7, 2025
20 checks passed
@dixonjoel dixonjoel deleted the users/jdixon/reuse-analyze-project-action branch October 7, 2025 17:45
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.

4 participants