-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Update self hosting docs #6877
Merged
Merged
Update self hosting docs #6877
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Refs #6854
I think we can probably just remove this note. As I understand it, in order to need/use this, a user would have to set up their own GitHub OAuth app to collect tokens for their pool. I think this situation is so specialised to our instance that we can just remove this note rather than documenting the process for running a token pool. Does this seem reasonable?
All we say about
GH_CLIENT_ID
andGH_CLIENT_SECRET
in the server-secrets docs is "These settings are used by shields.io for GitHub OAuth app authorization but will not be necessary for most self-hosted installations"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.
Isn't the OAuth just a means for soliciting tokens and then filling the pool though? On the off chance a self-hoster did find themselves hitting Hub's 5k hourly limit, I think they'd be able to resolve by getting one or two users (presumably another member of their team) to provide a token which they could stash in Redis
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 is, but I don't think we provide any alternative way to populate the pool, do we?
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.
Let me know if my understanding is off, or if I'm just missing something, but I'm trying to make a distinction between the Shields server loading the tokens from the configured Redis instance, separately from how the tokens get into Redis in the first place.
I know our server also provides the means to accept donated tokens and add those into Redis, but my understanding is that the piece that initializes/loads the tokens is actually agnostic of the mechanism used to insert the tokens to the Redis store. I.e., if I were to manually create a new token and use a script/admin console/whatever to add my token to our Redis instance on Compose, that'd be just like all the other tokens and pulled into the Shields runtime where the server wouldn't care how that token originally got into Redis, wouldn't it?
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.
Yes. If you wrote your own script to insert tokens into a redis instance, that would work (I've not actually tested it), but we don't provide one. The pooling code doesn't care how the tokens got there.
Do you think that is a scenario we want to explicitly document for self-hosting users?
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 think I'm okay with the proposed changes as-is since it's highly unlikely anyone would need it and the current text can obviously be a source of confusion for users. I just wanted to get clarity on whether the token pool was tightly coupled to the OAuth app approach