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: auto inject Bitrise default host rule #30490

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

secustor
Copy link
Collaborator

Changes

Injects new hostRules scoped to the bitrise hostType and matchHost https://api.github.com/repos/bitrise-io/bitrise-steplib/contents.

Context

Currently, by default no authorization header are added while looking up Bitrise data, which results in rate limiting for bigger runs.

This is an alternative to setting the hostType to github.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@secustor secustor requested review from viceice and rarkins July 30, 2024 21:50
viceice
viceice previously approved these changes Sep 30, 2024
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

I'm still concerned that it will break exising users

@secustor secustor enabled auto-merge October 2, 2024 20:17
Comment on lines 33 to 42
```json title="Host Rule which matches the Bitrise step lib repository and datasource"
{
"hostRules": [
{
"hostType": "bitrise",
"matchHost": "https://api.github.com/repos/my-org/my-repo/contents"
}
]
}
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like having a host rule in docs which "does nothing". Would adding a dummy token here be appropriate?

viceice
viceice previously approved these changes Dec 12, 2024
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

I still think this can cause harm 🤷‍♂️

@secustor
Copy link
Collaborator Author

What do you expect to break / cause harm? 🤔

The token is scoped to the repository.

@viceice
Copy link
Member

viceice commented Dec 12, 2024

What do you expect to break / cause harm? 🤔

The token is scoped to the repository.

I'm not sure. it's a feeling.

@secustor secustor requested a review from rarkins January 18, 2025 20:12
@secustor secustor closed this Jan 31, 2025
auto-merge was automatically disabled January 31, 2025 10:51

Pull request was closed

@secustor secustor deleted the feat/inject-bitrise-auto-host-rule branch January 31, 2025 10:51
@secustor secustor restored the feat/inject-bitrise-auto-host-rule branch January 31, 2025 10:51
@effektsvk
Copy link

@secustor Why was this closed? Is it not getting merged?

@secustor secustor reopened this Feb 1, 2025
@secustor
Copy link
Collaborator Author

secustor commented Feb 1, 2025

I have accidentally deleted the branch, while cleaning up my fork.

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.

4 participants