-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: master
Are you sure you want to change the base?
fix: accept platforms name contained equal sigh #2160
Conversation
Codecov ReportAttention:
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. |
28d8d90
to
c7ce869
Compare
c7ce869
to
157b0be
Compare
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.
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.
@wolfogre In my case, I use self-hosted-runners that are labeled the name runner-label 's specification accepts thanks. |
Sorry, I'm afraid I need to reserve my opinion. I don't see a strong reason to include For most command tools, $ 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) |
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" |
@wolfogre
I would like to avoid changing the runner-label name to use act if possible. |
How about modifying the content to accept the format of grouping the key string with any symbols. Would it look ugly? |
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 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.
What would we do about a currently valid label char like Before you opened the issue, I didn't even know that
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 |
@wolfogre Please re-review. |
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.
Can we have some unittests for this feature?
pParts = pParts[:len(pParts)-1] | ||
|
||
pName := strings.Join(pParts, "=") | ||
platforms[strings.ToLower(pName)] = pValue |
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.
Wondering if we can have tests for this? Not sure if there is a rule where you should add tests for each new feature.
I take a neutral stance on this. Please ignore my change request and let other maintainers decide. |
Fix #2159
I hope to help act user using GitHub EnterPrise Server.