-
-
Notifications
You must be signed in to change notification settings - Fork 747
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
Use codecov-cli over python-codecov #6036
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR to fix it!
Looks good,
I left one comment about verifying the coverage in this PR.
Please also update the Changelog with a note about using codecov-cli.
A couple of things to note in the PR. The python 3.6 tests seem to be failing due to the codecov-cli-0.1.13 being installed which was intresting, so I am going to try upgrading the version of pip before to see if that helps. The python 3.9 and python 3.8 versions seem to have some issue with getting the environment var
I don't see where this is being set in the code base or via the github action so I guess I'll try the |
Yeah after looking a bit further I now see where the env vars can be seen in github, and it's def not being passed from that end. If anyone know please just leave a comment, however, I do not know where to pull the codecov token from. On the topic of python 3.6. It seems that the tool does not have the |
Thanks for the commit, for my own docs in the future I didn't see this: (GitHub) Using secrets in a workflow. I'll probs try using the -t flag later on today to see if I can get this working. |
@FileMagic We indeed didn't have CODECOV_TOKEN added (generated) for this repository. I just added it, but looks like the main repository secrets are not accessible from the GH forks (codecov/codecov-action#44 (comment)) during the build. Could you try to generate a Codecov token and add it to your Github Repo (fork) just for testing purposes? |
Minor addition, based on https://github.com/StackStorm/st2/actions/runs/6564663313/job/17831515543?pr=6036#step:17:33
We may need to rename the |
It seems that even when |
I used a key for code cov here one time on a free account to test if it would work. It seems that it doesn't read from my project's github secrets either. I regenerated the key after so we should be good. However, we will see if this technically would have worked. |
Well, that seems to work as expected. I'll revert the change |
test-requirements.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I unpin the version of coverage here due to codecovcli not requiring it anymore directly. This can be seen here
The last thing I did was remove the extra coverage install from I tested the updated script (without coverage install) in my cli: Requirement already satisfied: pip in ./venv/lib/python3.9/site-packages (23.0)
Collecting pip
Downloading pip-23.3.1-py3-none-any.whl (2.1 MB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 2.1/2.1 MB 7.0 MB/s eta 0:00:00
Installing collected packages: pip
Attempting uninstall: pip
Found existing installation: pip 23.0
Uninstalling pip-23.0:
Successfully uninstalled pip-23.0
Successfully installed pip-23.3.1
info - 2023-10-21 19:26:39,462 -- ci service found: local
info - 2023-10-21 19:26:39,611 -- Process Commit creating complete
error - 2023-10-21 19:26:39,611 -- Commit creating failed: {"detail":"Invalid token."} So we should be good now. Let me know if you see anything else that should be changed. |
# Without eventlet being available to the coverage command it will fail with "Couldn't trace with concurrency=eventlet, the module isn't installed." | ||
pip install eventlet | ||
# NOTE: codecov only supports coverage==4.5.2 | ||
pip install 'coverage<5.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With removing coverage
(9f27017) in a bash script we get the following error at a later stage:
https://github.com/StackStorm/st2/actions/runs/6603759812/job/17937403741?pr=6040#step:17:38
info - 2023-10-22 12:48:45,887 -- ci service found: github-actions
info - 2023-10-22 12:48:45,991 -- Process Commit creating complete
info - 2023-10-22 12:48:46,101 -- Process Report creating complete
info - 2023-10-22 12:48:46,101 -- Finished creating report successfully --- {"response": "{\"external_id\":\"2aa2e37f-d821-4cad-9199-ecd04092be89\",\"created_at\":\"2023-10-22T12:35:18.103083Z\",\"commit_sha\":\"d1666b936f8e58a8beccc69ad8d4a1772f74b20b\",\"code\":null}"}
warning - 2023-10-22 12:48:46,102 -- xcrun is not installed or can't be found.
warning - 2023-10-22 12:48:46,429 -- No gcov data found.
warning - 2023-10-22 12:48:46,429 -- coverage.py is not installed or can't be found.
info - 2023-10-22 12:48:46,522 -- Found 0 coverage files to upload
Error: No coverage reports found. Please make sure you're generating reports successfully.
Error: Process completed with exit code 1.
so it's indeed needed, please keep it included
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will put it back one sec here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize about that. That should do it for ya.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, and thanks for your work! 👍
…cli" This reverts commit 456b61b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for fixing the st2 master build! 🟢
Unit Tests are running fine now and reporting the coverage 🟢 (https://app.codecov.io/gh/StackStorm/st2/commits) after merging this PR. The Orquesta Integration tests though failed 🔴 on: st2/.github/workflows/orquesta-integration-tests.yaml Lines 58 to 75 in d374b92
st2/.github/workflows/orquesta-integration-tests.yaml Lines 227 to 231 in d374b92
where we missed to include the
@FileMagic If you'd like to help with that as well, considering you already figured out the solution in this PR. |
I see, yeah I can fix that tonight. |
This is the PR related to Issue #6035