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

feat(api): support officially documented generic CI env vars (#299) #300

Merged
merged 3 commits into from Jul 20, 2021

Conversation

abravalheri
Copy link
Contributor

@abravalheri abravalheri commented Jun 3, 2021

This PR is related to #299 and #256.

The official docs for Coveralls define a set of generic environment variables that once defined should support any CI environment: https://docs.coveralls.io/supported-ci-services (bottom of the page).

The changes implemented in this PR attempt to read those variables but allows the values to be overwritten by any CI-specific configuration currently supported. They were also inspired by the official client (coveralls-ruby - lib/coveralls/configuration.rb -set_standard_service_params_for_generic_ci).

On top of the automated tox and pre-commit, I also did a manual test (to make sure no coveralls API error was introduced) following the steps described bellow:

  1. Run coveralls debug in an existing project/CI and copied the printed JSON string
  2. Add the following keys and associated values regarding a non-supported CI service (e.g. cirrus-ci) to the copied JSON:
    service_name
    service_number
    service_build_url
    service_job_id
    service_branch
    service_pull_request
    repo_token
    which include all the configuration keys being used in the new function load_config_generic_ci_environment
  3. Manually submit the modified JSON payload to https://coveralls.io/api/v1/jobs, using the requests library, using:
    requests.post('https://coveralls.io/api/v1/jobs', files={'json_file': json.dumps(payload)}, 
    verify=False)

The POST request was successful (200 return code), and the outcome can be observed in https://coveralls.io/builds/40277688, which seem to indicate that the changes do not introduce any non-supported payload key for the coveralls HTTP API (which was already expected since it mimics the official Ruby client implementation).

Please let me know of any feedback, I can also implement some changes if there is something that is not adequate.

A final observation about the changes in the docs: although the official webpage does not describe the meaning of each CI_ environment variable, I managed to create some useful description by inspect both Ruby and Python clients and cross-correlating the environment variables for other services according to their documentation (e.g. Travis, Circle, GitLab, GitHub, Semaphore).

Copy link
Owner

@TheKevJames TheKevJames left a comment

Choose a reason for hiding this comment

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

Hi @abravalheri -- thanks for the contribution and sorry I've fallen so far behind on reviews. Really appreciate the work put in here to track down descriptions for each of these!

Overall this LGTM, just a question around a difference between your implementation and the ruby one.

coveralls/api.py Show resolved Hide resolved
@abravalheri
Copy link
Contributor Author

Thank you very much for the review @TheKevJames.

I tried to reply to your question with some information, but if you prefer to make the implementation more similar to the Ruby one, please let me know, I can also do that.

The official docs for Coveralls define a set of generic environment
variables that once defined should support any CI environment:

https://docs.coveralls.io/supported-ci-services (bottom of the page).

The changes implemented in this PR attempt to read those variables
but allows the values to be overwritten by any CI-specific configuration
currently supported. They were also inspired by the official client
(coveralls-ruby - lib/coveralls/configuration.rb -
set_standard_service_params_for_generic_ci).
@abravalheri
Copy link
Contributor Author

Hi @TheKevJames, I (hopefully) improved the docs about CI_PULL_REQUEST and also rebased the latest changes from the master.

Please let me know if any other changes are required.

Copy link
Owner

@TheKevJames TheKevJames left a comment

Choose a reason for hiding this comment

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

@abravalheri perfect, the updated docs look great. Thanks again for the contribution!

@TheKevJames TheKevJames merged commit ca1c6a4 into TheKevJames:master Jul 20, 2021
@abravalheri abravalheri deleted the add-generic-ci branch July 20, 2021 20:15
andy-maier pushed a commit to andy-maier/coveralls-python that referenced this pull request Dec 23, 2022
…ames#299) (TheKevJames#300)

The official docs for Coveralls define a set of generic environment
variables that once defined should support any CI environment:

https://docs.coveralls.io/supported-ci-services (bottom of the page).

The changes implemented in this PR attempt to read those variables
but allows the values to be overwritten by any CI-specific configuration
currently supported. They were also inspired by the official client
(coveralls-ruby - lib/coveralls/configuration.rb -
set_standard_service_params_for_generic_ci).
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.

None yet

2 participants