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

fix: accept platforms name contained equal sigh #2160

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

totechite
Copy link

@totechite totechite commented Jan 19, 2024

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

@totechite totechite requested a review from a team as a code owner January 19, 2024 20:59
Copy link

codecov bot commented Jan 20, 2024

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 ⚠️
pkg/runner/run_context.go 73.37% 75 Missing and 19 partials ⚠️
pkg/runner/expression.go 55.17% 66 Missing and 12 partials ⚠️
pkg/runner/action_cache.go 50.74% 49 Missing and 17 partials ⚠️
pkg/container/docker_network.go 0.00% 56 Missing ⚠️
pkg/container/docker_run.go 1.92% 50 Missing and 1 partial ⚠️
pkg/model/workflow.go 43.37% 40 Missing and 7 partials ⚠️
pkg/common/outbound_ip.go 0.00% 44 Missing ⚠️
pkg/container/host_environment.go 0.00% 43 Missing ⚠️
pkg/runner/reusable_workflow.go 42.02% 35 Missing and 5 partials ⚠️
... 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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@totechite totechite force-pushed the fix/accept_runnerLabel_containEqualSigh branch from 28d8d90 to c7ce869 Compare January 20, 2024 05:01
@totechite totechite force-pushed the fix/accept_runnerLabel_containEqualSigh branch from c7ce869 to 157b0be Compare January 20, 2024 05:02
Copy link
Member

@wolfogre wolfogre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have read #2159 carefully, however, I didn't see a strong reason to support = in the platform name. Could you please explain why it will help users using GitHub Enterprise Server? Why is = required? TBH, I think IMAGE=debian-12=nektos/act-environments-ubuntu:22.04 is quite confusing.

@mergify mergify bot requested a review from a team January 24, 2024 02:21
@totechite
Copy link
Author

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

@wolfogre
Copy link
Member

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)

@ChristopherHX
Copy link
Contributor

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"

@totechite
Copy link
Author

totechite commented Jan 27, 2024

@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
Copy link
Author

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

@ChristopherHX
Copy link
Contributor

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

@totechite
Copy link
Author

@wolfogre Please re-review.

Copy link

@allexiusw allexiusw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have some unittests for this feature?

pParts = pParts[:len(pParts)-1]

pName := strings.Join(pParts, "=")
platforms[strings.ToLower(pName)] = pValue

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we can have tests for this? Not sure if there is a rule where you should add tests for each new feature.

@wolfogre
Copy link
Member

@wolfogre Please re-review.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Reject platforms name contained "="
4 participants