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: introduce dedicated option for GitHub API endpoint #829

Merged
merged 3 commits into from
May 15, 2024

Conversation

fgreinacher
Copy link
Contributor

@fgreinacher fgreinacher commented May 10, 2024

Server and API endpoints can be different. Some code parts, e.g. issue parsing, need the server endpoint and we should make this explicit. The existing githubApiPathPrefix option is not sufficient when the API is served from a different host (like api.github.com).

Fixes #827

⚒️ with ❤️ by Siemens

@fgreinacher fgreinacher marked this pull request as draft May 10, 2024 06:19
Comment on lines +74 to +79
| Variable | Description |
| ------------------------------ | ------------------------------------------------------------------- |
| `GITHUB_TOKEN` or `GH_TOKEN` | **Required.** The token used to authenticate with GitHub. |
| `GITHUB_URL` or `GH_URL` | The GitHub server endpoint. |
| `GITHUB_PREFIX` or `GH_PREFIX` | The GitHub API prefix, relative to `GITHUB_URL`. |
| `GITHUB_API_URL` | The GitHub API endpoint. Note that this overwrites `GITHUB_PREFIX`. |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Independent of this change it might also be worth to deprecate some of these variables to align with what is currently supported: https://docs.github.com/en/actions/learn-github-actions/variables.

Server and API endpoints can be different. Some code parts, e.g. issue parsing, need the server endpoint and we should make this explicit.
@fgreinacher fgreinacher requested a review from gr2m May 10, 2024 06:25
@fgreinacher fgreinacher marked this pull request as ready for review May 10, 2024 06:25
@fgreinacher fgreinacher requested a review from travi May 10, 2024 06:40
@travi
Copy link
Member

travi commented May 10, 2024

thanks for reporting and investigating. i hadnt noticed this behavior, but see it in other releases now that i'm looking for it. i'm unsure when it began or what it might be related to.

i havent investigated very far yet, but it seems odd to me to need to adjust for this. since octokit is used for the interactions with github, most of the differences between using github.com and a self-hosted github enterprise server instance should be covered by the existing config options. this feels more like a regression or octokit api mismatch somewhere to me, but would need to investigate further to understand better.

i'm curious if @gr2m is aware of anything obvious that might be contributing to this situation

@gr2m
Copy link
Member

gr2m commented May 13, 2024

I'll try to have a look this week

@fgreinacher
Copy link
Contributor Author

fgreinacher commented May 13, 2024

thanks for reporting and investigating. i hadnt noticed this behavior, but see it in other releases now that i'm looking for it. i'm unsure when it began or what it might be related to.

i havent investigated very far yet, but it seems odd to me to need to adjust for this. since octokit is used for the interactions with github, most of the differences between using github.com and a self-hosted github enterprise server instance should be covered by the existing config options. this feels more like a regression or octokit api mismatch somewhere to me, but would need to investigate further to understand better.

i'm curious if @gr2m is aware of anything obvious that might be contributing to this situation

I'm quite sure it's not an Octokit (configuration) issue. The regular API requests via Octokit are working fine. The problem occurs when the plugin code is filtering detected issue references:

  1. the plugin code is configuring the underlying issue-parser library with hosts = [gitHubUrl]: https://github.com/semantic-release/github/blob/master/lib/success.js#L56.
  2. gitHubUrl contains the GitHub API URL (api.github.com)
  3. the issue-parser library uses that URL to derive the slug: https://github.com/semantic-release/issue-parser/blob/master/index.js#L52.
  4. that slug is wrong when the input text contains full URLs as issue references (e.g. https://github.com/semantic-release/github/issues/827)
  5. the GitHub plugin uses the slug to find relevant issue references: https://github.com/semantic-release/github/blob/master/lib/success.js#L116
  6. with the wrong slug it will filter out full URL issue references and not comment on any of them

TLDR: issue-parser needs the root GitHub URL, octokit needs the GitHub API URL, and we do not disambiguate these currently.


I guess this broke in #269, with it's 4th birthday tomorrow :)

Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Outstanding pull request, thank you @fgreinacher 💐 Changes and reasoning all look good to me, and code + tests are great 👍🏼

@gr2m gr2m enabled auto-merge (squash) May 15, 2024 22:26
@gr2m gr2m merged commit 908ff83 into semantic-release:master May 15, 2024
6 checks passed
Copy link

🎉 This PR is included in version 10.0.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing success comments on issues
3 participants