Clarify labels #69

Merged
wolfogre merged 6 commits from feature/host_mode into main 2023-03-23 12:48:33 +00:00
wolfogre commented 2023-03-23 08:59:31 +00:00 (Migrated from gitea.com)

The label will follow the format label[:schema[:args]], and the schema will be host if it's omitted. So

  • ubuntu:docker://node:18: Run jobs with label ubuntu via docker with image node:18
  • ubuntu:host: Run jobs with label ubuntu on the host directly.
  • ubuntu: Same as ubuntu:host.
  • ubuntu:vm:ubuntu-latest: (Just a example, not Implemented) Run jobs with label ubuntu via virtual machine with iso ubuntu-latest.
The label will follow the format `label[:schema[:args]]`, and the schema will be `host` if it's omitted. So - `ubuntu:docker://node:18`: Run jobs with label `ubuntu` via docker with image `node:18` - `ubuntu:host`: Run jobs with label `ubuntu` on the host directly. - `ubuntu`: Same as `ubuntu:host`. - `ubuntu:vm:ubuntu-latest`: (Just a example, not Implemented) Run jobs with label `ubuntu` via virtual machine with iso `ubuntu-latest`.
wxiaoguang (Migrated from gitea.com) reviewed 2023-03-23 09:15:45 +00:00
@@ -0,0 +45,4 @@
}
switch schema {
case "docker":
wxiaoguang (Migrated from gitea.com) commented 2023-03-23 09:15:45 +00:00

There is a if len(splits) != 1 { above?

There is a `if len(splits) != 1 {` above?
wolfogre (Migrated from gitea.com) reviewed 2023-03-23 09:18:56 +00:00
@@ -0,0 +45,4 @@
}
switch schema {
case "docker":
wolfogre (Migrated from gitea.com) commented 2023-03-23 09:18:56 +00:00

Typo. 😂

Typo. 😂
wxiaoguang (Migrated from gitea.com) reviewed 2023-03-23 09:23:37 +00:00
@@ -0,0 +53,4 @@
default:
// It should not happen, because ParseLabel has checked it.
continue
}
wxiaoguang (Migrated from gitea.com) commented 2023-03-23 09:23:37 +00:00

Does it need a default case? At least, show some error logs.

Does it need a `default` case? At least, show some error logs.
Zettat123 (Migrated from gitea.com) approved these changes 2023-03-23 09:24:35 +00:00
@@ -0,0 +38,4 @@
func (s *Runner) platformPicker(labels []string) string {
platforms := make(map[string]string, len(s.Labels))
for _, l := range s.Labels {
label, schema, arg, err := ParseLabel(l)
Zettat123 (Migrated from gitea.com) commented 2023-03-23 09:24:35 +00:00

should be make(map[string]string, len(s.Labels)) ?

should be `make(map[string]string, len(s.Labels))` ?
wxiaoguang (Migrated from gitea.com) reviewed 2023-03-23 09:29:50 +00:00
@@ -59,3 +46,3 @@
INFO Enter the runner name (if set empty, use hostname: Test.local):
INFO Enter the runner labels, leave blank to use the default labels (comma-separated, for example, ubuntu-latest:docker://gitea/runner-images:ubuntu-latest):
INFO Enter the runner labels, leave blank to use the default labels (comma-separated, for example, ubuntu-20.04:docker://node:16-bullseye,ubuntu-18.04:docker://node:16-buster,linux_arm:host):
wxiaoguang (Migrated from gitea.com) commented 2023-03-23 09:29:51 +00:00

I guess it could have some more explanations about each part of the "label" .

And it looks strange that linux_arm:host becomes -self-hosted

I guess it could have some more explanations about each part of the "label" . And it looks strange that `linux_arm:host` becomes `-self-hosted`
ChristopherHX commented 2023-03-23 09:30:16 +00:00 (Migrated from gitea.com)

Previously, the only way to register a runner for running jobs on a host without containers was to use the label "self-hosted". However, this design was ill-conceived.

This limitation doesn't exists, only documentation wise.

linux_arm as labelstring is also host mode.

> Previously, the only way to register a runner for running jobs on a host without containers was to use the label "self-hosted". However, this design was ill-conceived. This limitation doesn't exists, only documentation wise. `linux_arm` as labelstring is also host mode.
wolfogre commented 2023-03-23 09:43:51 +00:00 (Migrated from gitea.com)

Previously, the only way to register a runner for running jobs on a host without containers was to use the label "self-hosted". However, this design was ill-conceived.

This limitation doesn't exists, only documentation wise.

linux_arm as labelstring is also host mode.

I got it.

I noticed this problem when writing docs, since the original design of complete label is label:schema[:args].
So ubuntu:docker://node:18, and maybe ubuntu:vm:xxxx, ubuntu:cri:xxxx in the future.
That's why I think it should be ubuntu:host. The host is schema.

However, @wxiaoguang has a better idea that the schema can be optional and is "-self-hosted" by default, like label[:schema[:args]].

It makes sense, I will close this PR.

I will rewrite this PR.

> > Previously, the only way to register a runner for running jobs on a host without containers was to use the label "self-hosted". However, this design was ill-conceived. > > This limitation doesn't exists, only documentation wise. > > `linux_arm` as labelstring is also host mode. I got it. I noticed this problem when writing docs, since the original design of complete label is `label:schema[:args]`. So `ubuntu:docker://node:18`, and maybe `ubuntu:vm:xxxx`, `ubuntu:cri:xxxx` in the future. That's why I think it should be `ubuntu:host`. The `host` is schema. However, @wxiaoguang has a better idea that the schema can be optional and is "-self-hosted" by default, like `label[:schema[:args]]`. It makes sense, ~~I will close this PR.~~ I will rewrite this PR.
ChristopherHX commented 2023-03-23 09:58:30 +00:00 (Migrated from gitea.com)

However, @wxiaoguang has a better idea that the schema can be optional and is "-self-hosted" by default, like label[:schema[:args]].

Yes this would avoid a breaking change. I had choosen to not add a new schema, because host mode doesn't need any args. However it is a valid idea to add one.

Maybe https://github.com/nektos/act/pull/1682 will get an own schema like "lxc://".

> However, @wxiaoguang has a better idea that the schema can be optional and is "-self-hosted" by default, like `label[:schema[:args]]`. Yes this would avoid a breaking change. I had choosen to not add a new schema, because host mode doesn't need any args. However it is a valid idea to add one. Maybe https://github.com/nektos/act/pull/1682 will get an own schema like "lxc://".
wolfogre commented 2023-03-23 10:19:44 +00:00 (Migrated from gitea.com)

@ChristopherHX I rewrote this PR, could you please take a look at it again.

@ChristopherHX I rewrote this PR, could you please take a look at it again.
ChristopherHX (Migrated from gitea.com) approved these changes 2023-03-23 11:23:21 +00:00
ChristopherHX (Migrated from gitea.com) left a comment

Works perfectly fine. I see you have removed the label parsing code I had duplicated in my change.

Works perfectly fine. I see you have removed the label parsing code I had duplicated in my change.
wxiaoguang (Migrated from gitea.com) approved these changes 2023-03-23 11:49:31 +00:00
lunny (Migrated from gitea.com) approved these changes 2023-03-23 12:49:14 +00:00
Sign in to join this conversation.