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

extract act options from cli command #117

Conversation

ChristopherHX
Copy link
Contributor

@ChristopherHX ChristopherHX commented Dec 14, 2024

✍ Changes

Call --list-options and use this for the add option dialog if the command succeeds.

πŸ“‹ Checklist

  • I tested my changes with Runner.Client (but no unit tests)
  • I tested my changes with nektos/act PR (not yet)
  • I updated relevant documentation (N/A, no user facing change)

See options not available in nektos/act listed now using this flag as the cli tools knows them
image

Some problems I see are

  • your free style options dialog cannot make use of the stringArray vs string types yet.
  • for options of type bool, we should not have a free text input and use empty string "" or "false" as only selectable values
  • options are not validated using --list-options if it succeeds (e.g. marked in red / ignored / show an error or whatever)
  • eventually cache the result, a small delay has been added by this change

For your mentioned descriptions updates please just file a PR to change these here: https://github.com/nektos/act/blob/9c7f103bb39f11430f58c1a68d5629a42e6d56a1/cmd/root.go#L47-L106

It's unusual for me to see the Merge Commit merge strategy, we commonly use squash merge to only have one commit per pr in the default branch which is signed by GitHub instead of preserving unsigned commits

@ChristopherHX
Copy link
Contributor Author

ChristopherHX commented Dec 14, 2024

Eventually it is worth to filter out some special options like var etc.? So adding a filter before the map

Done, please squash merge if you want to go with this change

@ChristopherHX
Copy link
Contributor Author

ChristopherHX commented Dec 14, 2024

act tested as well
image

not all option defaults tested...

Known problem
- default value [] from cobra for some array options seems to be invalid, is a shell escaping issue in your extension works in double quotes

@SanjulaGanepola
Copy link
Owner

Some problems I see are

  • your free style options dialog cannot make use of the stringArray vs string types yet.
  • for options of type bool, we should not have a free text input and use empty string "" or "false" as only selectable values
  • options are not validated using --list-options if it succeeds (e.g. marked in red / ignored / show an error or whatever)
  • eventually cache the result, a small delay has been added by this change

Can you open issues to track this?

For your mentioned descriptions updates please just file a PR to change these here: https://github.com/nektos/act/blob/9c7f103bb39f11430f58c1a68d5629a42e6d56a1/cmd/root.go#L47-L106

Sounds good. I will make updates there.

It's unusual for me to see the Merge Commit merge strategy, we commonly use squash merge to only have one commit per pr in the default branch which is signed by GitHub instead of preserving unsigned commits

Good point. I have disabled Merge Commit strategy in favour of using Squash and Merge.

Copy link
Owner

@SanjulaGanepola SanjulaGanepola left a comment

Choose a reason for hiding this comment

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

@ChristopherHX I have some suggested changes here: ChristopherHX#1

for options of type bool, we should not have a free text input and use empty string "" or "false" as only selectable values

In that PR, I simply removed all bool type descriptions. I don't think we need any free text input or selectable values as simply having the option added and checked in the Settings view should be enough to mean it is being used.

- default value [] from cobra for some array options seems to be invalid, is a shell escaping issue in your extension works in double quotes

I am noticing some issues with the stringArray type options as well. How are the stringArray type options supposed to be passed in exactly?

@ChristopherHX
Copy link
Contributor Author

I don't think we need any free text input or selectable values as simply having the option added and checked in the Settings view should be enough to mean it is being used.

No you need false as well, look at the --pull option I might broke a boolean flag rule that --pull is noop and --pull=false is needed to opt out.

I am noticing some issues with the stringArray type options as well. How are the stringArray type options supposed to be passed in exactly?

String array should be passed like --env k1=val1 --env k2=val2, simple repeat the option and it's value. There are other possibilities (multiple syntaxes for the same thing) where I'm not sure about compatibility between act and Runner.Client.

In act you can repeat any option, one of them (I guess the last one) wins

* Refactor get options logic
* Remove bool type options from having descriptions
* Force option description to be uppercase

Signed-off-by: Sanjula Ganepola <[email protected]>
@ChristopherHX
Copy link
Contributor Author

I reverted the tested check boxes, since this is no longer the case...

@ChristopherHX
Copy link
Contributor Author

I am noticing some issues with the stringArray type options as well

Fixed this by removing the default value for stringArray type, a list can be defaulted to be always empty as those are appended anyway by repeating the option

Created a new fr issue as #129, for subtrees.

@SanjulaGanepola
Copy link
Owner

I am going to merge this and then make some additional fixes in a new PR. Thank you

@SanjulaGanepola SanjulaGanepola merged commit dc301e4 into SanjulaGanepola:main Jan 18, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants