-
Couldn't load subscription status.
- Fork 0
Re-use analyze-project from the python-actions repo #160
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
Conversation
|
@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 |
|
@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.) |
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 ( |
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. |
…nstead of having to list out every matrix combination
…alyze-project-action
…al version) to test if this PR will pass
|
@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? |
…alyze-project-action
79252d8 to
0956012
Compare
What does this Pull Request accomplish?
Uses the newly created
analyze-projectactions 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.