feature: Enable runner targeting by labels specified in runner profile #3145
Conversation
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.
Just a few nitpicks. Approved!
@@ -106,6 +109,9 @@ func (c *RunnerProfileInspectCommand) Run(args []string) int { | |||
{ | |||
Name: "Target Runner ID", Value: targetRunner, | |||
}, | |||
{ | |||
Name: "Target Runner Labels", Value: targetRunnerLabels, | |||
}, |
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.
Not a big deal, but a target runner can have only one of any, id, or lables, so should this be one field that is like Target:
instead that follows the format of list
or something?
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.
Hmm that's a good point actually, right now it's not hard-coded to restrict only one of ID or Labels (so you could define both), but ID always takes precedence over labels. I think I was thinking at one point "if they define both, but we can't find the runner by ID, then we take the labels instead". But since we ended up removing that "runner with this ID" exists check, we should allow only ID or Label, not both.
@@ -1,6 +1,7 @@ | |||
package cli | |||
|
|||
import ( | |||
"encoding/json" |
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.
Nitpick, across the project we typically do the pattern below, so we should have a newline between this and the next imports. Lots of tools automatically insert and break this once in awhile, not a big deal usually just manually clean it up.
import (
stdlib
external
internal
)
internal/cli/runner_profile_set.go
Outdated
Name: "target-runner-label", | ||
Target: &c.flagTargetRunnerLabels, | ||
Usage: "Labels on the runner to target for this remote runner profile. " + | ||
"e.g. `-target-runner-labels=k=v`. Can be specified multiple times.", |
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.
plural to singular here
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.
Looks pretty good to me! 👍🏻 I'll let Mitchell chime in since he was following the previous puzzle pieces, but overall makes sense to me. (edit: he beat me by 5 minutes! 😂 )
f.StringMapVar(&flag.StringMapVar{ | ||
Name: "target-runner-label", | ||
Target: &c.flagTargetRunnerLabels, | ||
Usage: "Labels on the runner to target for this remote runner profile. " + |
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.
Maybe this could use an additional sentence on what labels for targeting is used for?
PluginType: "magic-carpet", | ||
PluginConfig: []byte("foo = 1"), | ||
EnvironmentVariables: map[string]string{ | ||
"CARPET_DRIVER": "apu", |
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.
34308c1
to
b58a875
Compare
Another piece of the targetable runners puzzle.
This PR adds tests for job successfully queued and targeted, and CLI changes to allow creation of runner profiles with labels.
It also makes an important choice to allow definition of a runner profiles that targets a runner that does not exist yet. (Previous iteration of targetable runners did not allow this.)