-
Notifications
You must be signed in to change notification settings - Fork 1.2k
docs: improving docs #303
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
docs: improving docs #303
Conversation
@callum-tait-pbx Hey! I'm currently reviewing this carefully but it looks good so far. Awesome work! 👍
Did you mean to make the repository optional in |
yeh, it's not nice having to maintain the same list in 2 locations. Could be done either way, either:
The latter is probably the cleaner approach as doesn't introduce optional attributes. Optional attributes just introduces complexity by increasing the number of ways the solution can be deployed, and in this instance, without any benefit. It just seems a bit mad maintaining the same list twice! |
I think this affects HRA + Organizational Runner + Webhook-based autoscaling. GitHub API doesn't provide us the way to efficiently calculate which repository uses which organizational runner and how busy they are. If the user knows that a organizational runner is used only on repositories A, B, C, they can list it under So I'd rather agree with the first option: |
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.
LGTM. I appreciate all your contributions @callum-tait-pbx!
@mumoshu Since the docs were originally done we've added a few scaling metrics and so I thought the docs could do we some love as they were getting a bit unwieldy. I still don't love them but I've rejigged them to be a bit better (hopefully!) so things like the metrics are more lego like in description.
Few questions:
# Additional N number of sidecar containers
comment, am I understanding this correctly. This is how you can bolt on N number of sidecar containers, whatever they may be?RunnerDeployment
or is this a list by accident? I'm not really sure why you would want to provide multiple scaling metrics tbh with the existing metrics but it's worth highlighting either way and recommending a single scaling metric if so. Maybe it was made a list so that down the road you may want multiple as metrics are added?TotalNumberOfQueuedAndInProgressWorkflowRuns
metric you need to maintain the same list of repositories in both the HRA as well as the RunnerDeployment themselves? I am sure the reason is because it is a required attribute in both constructs but I wanted to check. If so would be make sense to make the repositories an optional attribute for theRunnerDeployment
as I would expect typically you are using them with a HRA. I know you can deploy just aRunnerDeployment
meaning you would need to define them in that situation but the kind could have them as optional so you only define them in theRunnerDeployment
if you actually need to do so? Worth mentioning we are not looking to maintain a list of repositories so I haven't played with the metric much, I may be asking silly quesitons here.