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

Split up pythonapp.yml #1309

Closed
Molkree opened this issue Aug 3, 2021 · 5 comments · Fixed by #1552
Closed

Split up pythonapp.yml #1309

Molkree opened this issue Aug 3, 2021 · 5 comments · Fixed by #1552
Labels
good first issue Good for newcomers
Milestone

Comments

@Molkree
Copy link
Contributor

Molkree commented Aug 3, 2021

From #1299 (review)

pythonapp.yml is getting kind of large and we might want to consider splitting it up later, but I don't think that needs to be done right now.

I suggest to split into at least 23 files:

  • linting.yml
    Current linting matrix.

  • testing.yml
    Everything else (docs, tests, long tests, Windows tests).

  • cve_scan.yml
    CVE scan

Events should stay the same for both, but inputs should be only in testing.yml.

General notes:

  1. use actions/setup-python@v2 and not v1 everywhere
  2. capitalize step names: get cached python packages -> Get cached Python packages, etc
  3. pip is already being updated but maybe also update setuptools, it's not the latest in GitHub runners
  4. install wheel when you install cve-bin-tool because of stuff like this Using legacy 'setup.py install' for idna-ssl, since package 'wheel' is not installed.
  5. I think docs/CVE scan can be run on the latest Python version so just omit python-version line

Tests:

  1. Regular tests job (not long): can omit matrix.os because it's just ubuntu-latest. Unless we want to test on more than that?
  2. Run long tests on Python 3.9 and add 3.8 to regular tests (I guess that's how testing is done in this repo), switch Win tests to 3.9
    cve-bin-tool does not yet officially support 3.9 Test with Python 3.9 - look for deprecation #910 (comment) but this can be a step towards it?
  3. ACTIONS environment variable can be pulled to the workflow level, no need to list it 3 times
  4. PYTHONIOENCODING env variable in Win tests 🤔 Not sure what it is and where it is used, safe to delete?
  5. Run long tests as a cron job/run it on .py file changes/cli.pyspecifically/etc. Originally suggested by @BreadGenie because we have missed it after switching from NVD JSON to NVD API retrieval.
  6. It looks like Windows Python 3.10 tests might in fact be running on Python 3.9 inside conda, look into this...
    image
  7. Use bool type for longTests input variable when running manually.
  8. Switch to codecov/codecov-action@v2, v1 will be sunset on February 1 2022.
  9. Bump technote-space/get-diff-action.
@Molkree
Copy link
Contributor Author

Molkree commented Oct 19, 2021

@terriko, any comments on this? If you don't object I'd like to take this.

@terriko
Copy link
Contributor

terriko commented Oct 26, 2021

This sounds pretty good. One addition: I'd take the cve scan out of linting and make it a separate thing. Mostly because I don't think cve scan really counts as linting, but also because in general if you fail linting the problem is in your code and you're the sole owner of that fix before a PR can be merged, but if you fail a cve scan most of the time the problem is not in your code and it may make more sense to fix in a separate PR.

@terriko
Copy link
Contributor

terriko commented Jan 26, 2022

This is longer than I usually flag as a good first issue, but it's well-described, so I'm going to go ahead and put the flag on this and see if anyone's interested.

@terriko terriko added the good first issue Good for newcomers label Jan 26, 2022
@terriko terriko added this to the 3.1 milestone Jan 26, 2022
@terriko
Copy link
Contributor

terriko commented Jan 26, 2022

Note also: some of this has already been done (e.g. we support 3.9 now) so you can't follow the instructions blindly

@Molkree
Copy link
Contributor Author

Molkree commented Jan 26, 2022

I've actually almost finished this in the last couple of days and simply couldn't test properly due to the NVD API key issue. I'll submit the PR shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants