act icon indicating copy to clipboard operation
act copied to clipboard

fix: accept platforms name contained equal sigh

Open totechite opened this issue 1 year ago • 10 comments

Fix #2159 I hope to help act user using GitHub EnterPrise Server.

totechite avatar Jan 19 '24 20:01 totechite

Codecov Report

Attention: 893 lines in your changes are missing coverage. Please review.

Comparison is base (4989f44) 61.22% compared to head (157b0be) 60.78%. Report is 303 commits behind head on master.

Files Patch % Lines
pkg/artifactcache/handler.go 65.46% 102 Missing and 42 partials :warning:
pkg/runner/run_context.go 73.37% 75 Missing and 19 partials :warning:
pkg/runner/expression.go 55.17% 66 Missing and 12 partials :warning:
pkg/runner/action_cache.go 50.74% 49 Missing and 17 partials :warning:
pkg/container/docker_network.go 0.00% 56 Missing :warning:
pkg/container/docker_run.go 1.92% 50 Missing and 1 partial :warning:
pkg/model/workflow.go 43.37% 40 Missing and 7 partials :warning:
pkg/common/outbound_ip.go 0.00% 44 Missing :warning:
pkg/container/host_environment.go 0.00% 43 Missing :warning:
pkg/runner/reusable_workflow.go 42.02% 35 Missing and 5 partials :warning:
... and 26 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2160      +/-   ##
==========================================
- Coverage   61.22%   60.78%   -0.44%     
==========================================
  Files          46       53       +7     
  Lines        7141     8966    +1825     
==========================================
+ Hits         4372     5450    +1078     
- Misses       2462     3079     +617     
- Partials      307      437     +130     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 20 '24 00:01 codecov[bot]

@wolfogre Thank you for checking As you may know, GHE users can't pick the GH Managed runner service so, we necessarily use self-hosted-runner when using GH Actions.

In my case, I use self-hosted-runners that are labeled the name IMAGE=debian-12 at my organization. however, I can't use act in my workspace using GHE because of this issue. And I don't hope to modify the runner-label for using this tool.

runner-label 's specification accepts = so if act support self-hosted-runner platform, may need this change.

thanks.

totechite avatar Jan 25 '24 04:01 totechite

Sorry, I'm afraid I need to reserve my opinion. I don't see a strong reason to include = in the label name in your case.

For most command tools, a=b=c will be treated as a: b=c, like docker run --env a=b=c --label a=b=c. And act has many parameters with key-value format. I hope they could keep the same behavior to avoid confusion.

$ act -h | grep stringArray
      --container-cap-add stringArray                     kernel capabilities to add to the workflow containers (e.g. --container-cap-add SYS_PTRACE)
      --container-cap-drop stringArray                    kernel capabilities to remove from the workflow containers (e.g. --container-cap-drop SYS_PTRACE)
      --env stringArray                                   env to make available to actions with optional value (e.g. --env myenv=foo or --env myenv)
      --input stringArray                                 action input to make available to actions (e.g. --input myinput=foo)
      --matrix stringArray                                specify which matrix configuration to include (e.g. --matrix java:13
  -P, --platform stringArray                              custom image to use per platform (e.g. -P ubuntu-18.04=nektos/act-environments-ubuntu:18.04)
      --replace-ghe-action-with-github-com stringArray    If you are using GitHub Enterprise Server and allow specified actions from GitHub (github.com), you can set actions on this. (e.g. --replace-ghe-action-with-github-com =github/super-linter)
  -s, --secret stringArray                                secret to make available to actions with optional value (e.g. -s mysecret=foo or -s mysecret)
      --var stringArray                                   variable to make available to actions with optional value (e.g. --var myvar=foo or --var myvar)

wolfogre avatar Jan 26 '24 07:01 wolfogre

imo yaml config files really make sense, the cli usage is super complex and .actrc is not a good format.

I believe the following yaml, make all escaping rules pretty clear.

platforms:
  ["ubuntu-latest", "self-hosted"]: "test"

ChristopherHX avatar Jan 26 '24 22:01 ChristopherHX

@wolfogre I agree with your thoughts, but how should I interpret the message here, which is also mentioned in #2159 ? It seems to accept a=b: c.

[test workflow/demo] 🚧 Skipping unsupported platform -- Try running with `-P IMAGE=debian-12=...

I would like to avoid changing the runner-label name to use act if possible. This is a so useful tool, but when using GH Actions, I don't hope should not be forced to use label names that take into account the specifications of this tool .

totechite avatar Jan 27 '24 05:01 totechite

How about modifying the content to accept the format of grouping the key string with any symbols. Would it look ugly? 'a=b'=c

totechite avatar Jan 27 '24 05:01 totechite

I don't have a strong opinion against this change, but this change is blocked for now until the requested changes are resolved.

We also need to make , a separator of the key, to correctly map multi label self-hosted runner constructs in nektos/act. Then obviosly the question comes up, what if my autoscaler embeds a , into a single label

Gitea hacked something together to workaround the multi label bug of act, but only one single label definitions wins while multiple images are possible for them as of now.

'a=b'=c

What would we do about a currently valid label char like '? It's about as exotic as a = sign in a label.

Before you opened the issue, I didn't even know that = are used in GitHub Actions Auto scalers. Without autoscalers this label syntax doesn't make sense for me.

wolfogre working on Gitea Actions, might be in a different world due to autoscalers not beeing that common there yet. I believe that pure dynamic labels don't work in gitea unless the autoscaler has a list of all possible images in the label IMAGE=<image> construct.

It might be not be that easy to make a technical discussion together with act maintainers, I tried it once and it was pretty silent

ChristopherHX avatar Jan 28 '24 15:01 ChristopherHX

@wolfogre Please re-review.

totechite avatar Jan 31 '24 18:01 totechite

@wolfogre Please re-review.

I take a neutral stance on this. Please ignore my change request and let other maintainers decide.

wolfogre avatar Feb 18 '24 03:02 wolfogre