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

Feature request - Gradle Properties options #18

Closed
gildor opened this issue Jun 3, 2020 · 6 comments
Closed

Feature request - Gradle Properties options #18

gildor opened this issue Jun 3, 2020 · 6 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@gildor
Copy link

gildor commented Jun 3, 2020

To pass Gradle property as argument we have 2 choices:

  1. Use env syntax and Gradle convention for environment variables:
 uses: eskatos/gradle-command-action@v1
      with:
         env:
           ORG_GRADLE_PROJECT_someSecret: 1234
         arguments: build

The issue of this approach is that it doesn't support properties with dots (very common practice, because of properties file)

  1. Pass property as command-line argument:
 uses: eskatos/gradle-command-action@v1
    with:
        arguments: build -PsomeSecret=1234

Works fine until you have 1-2 property, but becomes issue if you have many of them, multiline support would help but it also have issues in YML, requires special syntax and not very convenient to use, it also easy to forgot slash in command

So I suggest adding support for gradle properties in yml syntax:

 uses: eskatos/gradle-command-action@v1
      with:
         properties:
            someSecret: 1234
         arguments: build

Tho, there is an issue with it, because key with dot requires escaping in YML:
some.secret should be escaped like:
"[some.secret]"

Still fine, but not perfect. There is another option. Support dot as yml separator:

properties:
  some:
     secret: 1234

For 1 simple property it may be overkill and escape will work just fine

But check this real-life example:

 - name: Build
      uses: eskatos/gradle-command-action@v1
      with:
        arguments: build --info -PsomeService.prod.apiKey=${{ secrets.SOME_SERVICE_API_KEY }} -PsomeService.prod.secret=${{ secrets.SOME_SERVICE_API_SECRET }} -PsomeService.prod.projectId=${{ secrets.SOME_SERVICE_PROJECT_ID }}

As you see, now it's not easy to follow what is going on. it will be better with nested properties syntax:

 - name: Build
      uses: eskatos/gradle-command-action@v1
      with:
        arguments: build
        properties:
           someService:
               prod:
                  apiKey: ${{ secrets.SOME_SERVICE_API_KEY }}
                  secret: ${{ secrets.SOME_SERVICE_API_SECRET }}
                  projectId: ${{ secrets.SOME_SERVICE_PROJECT_ID }}

So much better even if debug and dev environments are configured

Maybe it should be considered to do the same for system properties, which are also very common

If you think it reasonable, I may try to implement it and contribute

@eskatos
Copy link
Member

eskatos commented Jun 13, 2020

Hi @gildor,
Sorry for the late response.

I can see how this can be annoying. But then you would have to deal with more YAML, I'm not sure I'm fond of it. It's a tradeoff between long arguments and 'complex' YAML.

I'm not against such an addition and agree that system properties should be implemented too if we go down that route.

The behaviour when one sets the same property in arguments and properties should be clarified though.

@eskatos eskatos added enhancement New feature or request help wanted Extra attention is needed labels Jun 15, 2020
@gildor
Copy link
Author

gildor commented Jun 21, 2020

Thanks for reply

The behaviour when one sets the same property in arguments and properties should be clarified though.

Agree, probably properties should be added after build arguments, it makes implementation simple (no need to parse and alter arguments somehow) and allows debug what kind properties are passed to gradle in build logs (because properties would be converted to arguments)

t's a tradeoff between long arguments and 'complex' YAML.

It's true, not a huge fan of more yaml, but we have what we have, and more readable workflow file is with a clear separation between tasks arguments and build properties makes sense here

@eskatos
Copy link
Member

eskatos commented Jun 22, 2020

Agree, probably properties should be added after build arguments, it makes implementation simple (no need to parse and alter arguments somehow) and allows debug what kind properties are passed to gradle in build logs (because properties would be converted to arguments)

Converting the properties to arguments sounds good.
About precedence, I guess the choice is between prepending or appending to arguments.
IOW, yaml properties have precedence, or arguments have.
I guess the former, appending, is reasonable.

@gildor
Copy link
Author

gildor commented Jun 23, 2020

Yes, appending as simple and straight forward solution so properties have precedence

@vlsi
Copy link

vlsi commented Aug 7, 2020

One of the issues is GitHub actions runner does not provide access to YAML configuration. It supports literal values only, and even YAML lists are not supported yet.

On the other hand, the following might be not that bad (I'm not sure if multi-line arguments already work, but it can definitely be supported):

 - name: Build
      uses: eskatos/gradle-command-action@v1
      with:
        arguments: |
          build --info
          -PsomeService.prod.apiKey=${{ secrets.SOME_SERVICE_API_KEY }}
          -PsomeService.prod.secret=${{ secrets.SOME_SERVICE_API_SECRET }}
          -PsomeService.prod.projectId=${{ secrets.SOME_SERVICE_PROJECT_ID }}

or

 - name: Build
      uses: eskatos/gradle-command-action@v1
      with:
        arguments: build --info
        properties: |
          someService.prod.apiKey=${{ secrets.SOME_SERVICE_API_KEY }}
          someService.prod.secret=${{ secrets.SOME_SERVICE_API_SECRET }}
          someService.prod.projectId=${{ secrets.SOME_SERVICE_PROJECT_ID }}

@bigdaz
Copy link
Member

bigdaz commented Sep 30, 2021

@vlsi I really like your idea.
@gildor I've opened #88 to add support for multi-line arguments, and will close this issue in preference to that solution.

Please add a comment if you feel a structured properties approach has some clear advantages.
Thanks!

@bigdaz bigdaz closed this as completed Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants