search

Pix4D / cogito

Concourse resource for GitHub Commit Status and Google Chat notifications
MIT License
35 stars 14 forks source link

Support GitHub Enterprise (GHE) - attempt #2 #139

Closed wanderanimrod closed 9 months ago

wanderanimrod commented 10 months ago

pairing with @dev-jin

Why

This resource only supports setting status on repos hosted in public GitHub. The domain github.com and api.github.com was the only API endpoint to which this resource could post-commit statuses.

What

  1. Add the field github_api_endpoint to the resource's source config (optional)
  2. Make GitHubCommitStatusSink and ProdPutter use the new source config field when present. If not, keep defaulting to the public GitHub API. Making the github_api_endpoint config optional keeps the resource simple for the majority of users (who I assume use public GH) and also ensures backward compatibility when those users upgrade to a version of this resource that supports GHE
  3. Update the README to mention the new source config field github_api_endpoint.
  4. Housekeeping a. Remove the ability to override the GH API with the COGITO_GITHUB_API environment variable. Change tests that used this to use the new github_api_endpoint source config field. b. Minor changes like fixing typos, removing unused vars and imports, etc. See inline comments for details.

Tests

marco-m-pix4d commented 10 months ago

Hello @wanderanimrod happy to see your persistence! :-) Please be advised that we are under load in my team so it might take a while for a review.

wanderanimrod commented 9 months ago

Hello @wanderanimrod happy to see your persistence! :-) Please be advised that we are under load in my team so it might take a while for a review.

Hello @marco-m-pix4d , any chance this will get looked at anytime soon?

marco-m-pix4d commented 9 months ago

@aliculPix4D if you need help on how to proceed, please ping me on our chat.

aliculPix4D commented 9 months ago

Closing this PR in favor of: https://github.com/Pix4D/cogito/pull/143

Note that the first commit is the squash of all commits present in this PR and it gives the credit to both authors from this PR.

aliculPix4D commented 9 months ago

@wanderanimrod just a bit more context. Basically, the only reason I closed this PR is because it was created from your fork, and it was simpler for me to commit and test the changes in this repo. So creating a new PR with a branch in this repository simplified running the tests and especially creating Concourse pipelines for our acceptance tests.

wanderanimrod commented 9 months ago

Thanks for the explanation @aliculPix4D . Makes sense. Looking forward to #143 getting merged. This has been on our backlog for a long time. Glad to see it coming to an end.