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

docs: improving docs #303

Merged
merged 16 commits into from
Feb 26, 2021
Merged

docs: improving docs #303

merged 16 commits into from
Feb 26, 2021

Conversation

callum-tait-pbx
Copy link
Contributor

@callum-tait-pbx callum-tait-pbx commented Feb 12, 2021

@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:

  • What are the scopes needed for PAT auth for the repo level, I have added in work done in update needed scopes for organization #250 into this PR but it only covers orgs.
  • Feel free to chime in pros / cons to each scaling metric, the lists I've made were just I could come up with
  • The # 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?
  • Am I right with all of my comments in the additional tweaks section?
  • The metrics attribute is a list, does that mean in theory you can provide multiple scaling metrics for the 1 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?
  • As it stands when using the 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 the RunnerDeployment as I would expect typically you are using them with a HRA. I know you can deploy just a RunnerDeployment meaning you would need to define them in that situation but the kind could have them as optional so you only define them in the RunnerDeployment 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.

@mumoshu
Copy link
Collaborator

mumoshu commented Feb 14, 2021

@callum-tait-pbx Hey! I'm currently reviewing this carefully but it looks good so far. Awesome work! 👍

If so would be make sense to make the repositories an optional attribute for the RunnerDeployment

Did you mean to make the repository optional in HorizontalRunnerAutoscaler.Spec.Metrics[].RepositoryNames, so that the HRA can use RunnerDeployment.Spec.Repository by default? If so, that does make sense to me.

@callum-tait-pbx
Copy link
Contributor Author

callum-tait-pbx commented Feb 15, 2021

Did you mean to make the repository optional in HorizontalRunnerAutoscaler.Spec.Metrics[].RepositoryNames, so that the HRA can use RunnerDeployment.Spec.Repository by default? If so, that does make sense to me.

yeh, it's not nice having to maintain the same list in 2 locations. Could be done either way, either:

  • Make the RepositoryNames list optional with RunnerDeployment and mandatory with HRAs. They can be included in RunnerDeployment so you can run sets of RunnerDeployments without a HRA if you really want but excluded if your RunnerDeployment is backed by a HRA
  • Or do it the other way, remove RepositoryNames from the HRA spec and make it pull its list from the RunnerDeployment it is targeting instead. If this approach was taken I wouldn't even make RepositoryNames part of the HRA spec, from my knowledge of the solution I don't see any benefit being able to define the RepositoryNames list twice? Even if you could point 1 HRA at multiple RunnerDeployments I would still make it get its list from all the associated targets instead of having to maintain its own list.

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!

@mumoshu
Copy link
Collaborator

mumoshu commented Feb 16, 2021

remove RepositoryNames from the HRA spec and make it pull its list from the RunnerDeployment it is targeting instead. If this approach was taken I wouldn't even make RepositoryNames part of the HRA spec, from my knowledge of the solution I don't see any benefit being able to define the RepositoryNames 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 RepositoryNames so that the webhook-based autoscaler scales up the deployment on each event on A, B C.

So I'd rather agree with the first option: Make the RepositoryNames list optional with RunnerDeployment.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@callum-tait-pbx callum-tait-pbx marked this pull request as ready for review February 18, 2021 16:35
Copy link
Collaborator

@mumoshu mumoshu left a 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 mumoshu merged commit f987571 into actions:master Feb 26, 2021
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.

None yet

2 participants