-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Fixed HTTP 422 for Github Actions; Improved/added log output #385
Fixed HTTP 422 for Github Actions; Improved/added log output #385
Conversation
Details: * The API documentation of coveralls.io states in https://docs.coveralls.io/api-jobs-endpoint that 'service_job_id' is required and is a unique ID for the job. Experiments show that not providing it causes result submission to fail with HTTP 422 "Unprocessable Entity". The current code of coveralls-python sets 'service_job_id' to None/null when submitting to coveralls.io. This worked until recently, and now causes HTTP 422 "Unprocessable Entity". This PR fixes that by setting 'service_job_id' to GITHUB_RUN_ID. Note that the current code sets 'service_number' to GITHUB_RUN_ID as well, and that is not changed by this PR. * Added a description of how the various service* request parameters work, based on the API documentation of coveralls.io and experiments, which allowed to partly correct that documentation. * Changed the print() for resubmission of a result to become a log.warning() call so it shows up in the ouutput at the right position. * Added log.info() calls for the original result submission and for the submission of the finish request. Signed-off-by: Andreas Maier <andreas.r.maier@gmx.de>
I think this PR only fails due to the current failures on Python 3.5 and 3.6 (issue #374). |
# For compatibility, the service continues to be set to 'github-actions' | ||
# for now. Users can specify the service name manually to override | ||
# this. | ||
service = 'github-actions' |
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.
Actually service should be github
to integrate with Github properly.
service = 'github-actions' | |
service = 'github' |
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 agree, I just did not want to change this without being sure. I think that the discussion in lemurheavy/coveralls-public#1710 also argues for using "github".
# this. | ||
service = 'github-actions' | ||
|
||
job = os.environ.get('GITHUB_RUN_ID') |
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.
Can we change job ID to be GITHUB_JOB
it should be unique within a workflow, so it's better use it for different jobs.
job = os.environ.get('GITHUB_RUN_ID') | |
job = os.environ.get('GITHUB_JOB') |
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.
GITHUB_JOB
is the name of the workflow, not its ID.
GITHUB_RUN_ID
is an ID that is unique within Actions, but the same for all Actions jobs in the same workflow run. That allows coveralls.io to combine the jobs of a build.
self.config['service_job_id'] = new_id | ||
log.warning( |
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.
It seems to me that with the fixes in coveralls.io, it is no longer necessary to resubmit, nor would it help to do that with a different service_job_id if the first submission already now sets a service_job_id.
@andy-maier thank you for going down the GHA rabbithole and figuring this all out. I've just read through this PR, the linked coveralls-public details, etc and done some testing. I think this is the right track (setting both values the same), as is using I did notice that despite the Also curious on your change to add the |
This PR solved the issue for our repos. It is ready for review and merge. For details, see the commit message.
Note that the description that I added, of how coveralls.io treats the request parameters, is based on experiments. An official confirmation e.g. via discussion of lemurheavy/coveralls-public#1710 is still pending.
Note that this PR does not change the default service name of "github-actions", and I still needed to set the service to "github", either with the
--service=github
option or with theCOVERALLS_SERVICE_NAME: "github"
env var. The question of whether to use service name "github" or "github-actions" is still a bit open I guess. My personal experience is that it always requires "github", but the comment in the code says that "github-actions" was more common.Note that it is possible for coveralls users to address the issue without this PR, as I described in #252. Nevertheless, I think the issue should be solved in coveralls.