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

fix for parallel mode, flag_name & base_path #8

Merged
merged 11 commits into from
Nov 29, 2020

Conversation

johanneswilm
Copy link
Contributor

This adds two new options (flag_name and base_path) that work the same way as they do in the main coveralls action and it fixes the calculation of the reference that is being used in the parallel_finished call.

@marvin-w
Copy link

marvin-w commented Nov 28, 2020

@AndreMiras Without this PR this action is kind of not usable.

@johanneswilm Tested successfully. Thank you!

Copy link
Owner

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice job @johanneswilm!
Thanks for looking into it and thanks for updating tests as well.

I see some tests of this PR are failing because of PR restricted access to GITHUB_TOKEN from PR. I haven't thought about that one, but I think the changes look good still.
For records, the error was:

Running on Github Actions but GITHUB_TOKEN is not set.
Add "env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}" to your step config.

I need to think of a remedy for next time (suggestions welcomed).

Note, for future PR maybe try to squash your commits to keep neat commit log.
Luckily I can squash it upon merge, but I'd rather have it already tidied up in the PR itself

@AndreMiras AndreMiras merged commit 3c62ca4 into AndreMiras:develop Nov 29, 2020
@AndreMiras
Copy link
Owner

We got a regression on the COVERALLS_REPO_TOKEN feature after merging this.
The error is:

Trying submitting coverage with service_name: github...
'service_job_id'
ExitCode.FAILURE

Error details:
https://github.com/AndreMiras/coveralls-python-action/runs/1468738734
@johanneswilm any idea why this is happening?

@johanneswilm
Copy link
Contributor Author

I guess if you use the COVERALLS_REPO_TOKEN rather than the GITHUB_REPO_TOKEN then the job id is calculated differently? If supporting that is important to you, maybe try to reinstate the old way of calculating the job id (that wasn't working with the GITHUB_REPO_TOKEN).

@johanneswilm
Copy link
Contributor Author

@AndreMiras You probably need to create another option --coveralls-token to be able to distinguish between the two. Sorry, I didn't get that you actually wanted to use the coveralls token.

@AndreMiras
Copy link
Owner

AndreMiras commented Nov 29, 2020

I started looking into it and yes your changes may not be related to this feature being broken, but maybe to an API change on their side.
Yes I'll probably look into this explicit option see if it helps. Will check that a bit later, thanks
Edit:
It's something that started to happen with the upgrade from coveralls==2.1.2 to coveralls==2.2.0.
Will debug from there

@AndreMiras
Copy link
Owner

Fixed via 50af55e

AndreMiras added a commit that referenced this pull request Dec 1, 2020
These functions are not longer used, refs #8
AndreMiras added a commit that referenced this pull request Dec 1, 2020
This is a follow up for #8.
- DRY: shares default between CLI and functions
- `flag_name` default from `False` to `None` (type consistency)
- `base_path` default from `False` to `.` (type consistency)
AndreMiras added a commit that referenced this pull request Dec 1, 2020
This is a follow up for #8.
- DRY: shares default between CLI and functions
- `flag_name` default from `False` to `None` (type consistency)
- `base_path` default from `False` to `.` (type consistency)
AndreMiras added a commit that referenced this pull request Dec 1, 2020
This is a follow up for #8, leverages context manager to change
directory. Also note we no longer check for `os.path.exists()` as we
prefer to fail loudly on that one.
AndreMiras added a commit that referenced this pull request Dec 1, 2020
This is a follow up for #8, leverages context manager to change
directory. Also note we no longer check for `os.path.exists()` as we
prefer to fail loudly on that one.
@AndreMiras
Copy link
Owner

Made some follow up refactoring changing the default of the two flags you introduced. I hope I didn't introduce any bugs I haven't integration tested.
Basically it was bugging me to have the default being a boolean (False) for a value that could take a string at the end. So I got the default a bit more type consistent

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.

3 participants