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

Pass scripts arguments #20

Closed
wants to merge 15 commits into from

Conversation

jwickowski
Copy link

I need to pass scripts args into cake script.

it allow for the feature from issue 10

Copy link
Author

@jwickowski jwickowski left a comment

Choose a reason for hiding this comment

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

a question to readme file

README.md Outdated Show resolved Hide resolved
@ecampidoglio ecampidoglio self-requested a review October 13, 2020 06:55
Copy link
Member

@ecampidoglio ecampidoglio left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

While I think that this is a good effort in solving the problem of passing custom arguments to the Cake script, I do have a few concerns about using a general-purpose library to parse the arguments defined in the workflow.

The Cake executable has a well-defined syntax to pass arguments to a script:

dotnet cake --foo=bar --baz

A library like yargs, on the other hand, is very flexible and will try to parse almost anything you throw at it into a list of options. Consider these examples:

const parse = require('yargs-parser');
parse('-foo=bar');
parse('-foo bar');

Both these strings will result in an object that looks like this:

{
  _: [],
  f: true,
  o: [ true, 'bar' ]
}

Which is not what we expected.

Is there a way to configure yargs so that it follows a well-defined syntax when parsing a string of arguments?

src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
__tests__/main.test.ts Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ecampidoglio ecampidoglio linked an issue Oct 13, 2020 that may be closed by this pull request
README.md Show resolved Hide resolved
@jwickowski
Copy link
Author

We can change the behavior of yargs-parser.

a canple you cen see here:
` var parse = require("yargs-parser");

console.log(parse('-foo=bar')); // { _: [], f: true, o: [ true, 'bar' ] }
console.log(parse('-foo bar')); // { _: [], f: true, o: [ true, 'bar' ] }

var conf = {configuration: {'short-option-groups': false}};
console.log(parse('-foo=bar', conf)); //{ _: [], foo: 'bar' }
console.log(parse('-foo bar', conf)); //{ _: [], foo: 'bar' }`

Will it be fine?
I can add test cases and implementation for that, but I am not sure if it is everything.

@jwickowski
Copy link
Author

Hi.
The pull request is waiting.
Can you look again and tell me what should be changed to be accepted and merged?

@ecampidoglio
Copy link
Member

ecampidoglio commented Oct 30, 2020

I gave this some serious thought and I've arrived to the conclusion that this implementation is not the right way to go.

There are a couple of reasons for this:

  1. Cake has a well-defined syntax for passing script arguments. It appears the yargs doesn't have a way to define one specific format to parse the arguments with, which makes translating the user-defined string into a set of valid Cake parameters extremely brittle.

  2. Ideally, I would like the syntax for specifying script arguments to be a YAML sequence of key-value pairs:

    with:
      arguments:
        - name: value
        - foo: bar

Until it's possible to parse action inputs as lists, I'd like to offer a syntax that's as close as possible to that ideal.

Having said that, I didn't want to close this PR without providing an alternative solution. After a few attempts, I settled on doing what most actions that accept a list argument do: parse it as a multi-line string:

with:
  arguments: |
    name: value
    foo: bar

I've implemented this in 49c1c65 and it seems to work well. I haven't released a new version yet, but if you want to try out the new arguments parameter right away, you can do so by referencing the master branch in your workflow:

steps:
  - name: Run the Cake script
    uses: cake-build/cake-action@master
    with:
      arguments: |
        name: value
        foo: bar

Let me know how it works for you.

I'd like to thank you again for your contribution. Even if the code itself ultimately didn't make it into the project, it served me as inspiration for finally tackling this problem. 🙏

@ecampidoglio ecampidoglio removed their request for review October 30, 2020 16:25
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.

Allow to specify configuration argument as input
3 participants