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

Allow use in other actions #33

Closed
wants to merge 1 commit into from
Closed

Conversation

MSP-Greg
Copy link
Collaborator

@MSP-Greg MSP-Greg commented Mar 3, 2020

@eregon
Copy link
Member

eregon commented Mar 5, 2020

What other action would need to use like this and not simply as a separate step?

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Mar 5, 2020

setup-ruby-pkgs

Maybe code to create pre-compiled binaries, where (ideally) one job needs several Ruby installs?

@eregon
Copy link
Member

eregon commented Mar 5, 2020

Could you describe a bit how you would use it and what it would enable for setup-ruby-pkgs?

Just finding the Ruby prefix should be easy by shelling out to ruby -e 'print RbConfig::CONFIG["prefix"]'.

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Mar 5, 2020

First of all, I hate repetitive or unneeded things. Extra complexity is bad. And, yes, I don't always write code that way, although I try...

So, my thought was if you just need plain Ruby, one uses setup-ruby. If you want packages installed, compilers updated, etc, you use setup-ruby-pkgs. IOW, there would be no need to use both. Both have a ruby-version: input.

Obviously, setup-ruby-pkgs can be a separate action that would also require using setup-ruby. To me, that's 'extra complexity'...

As to how I'd use it, I've already done so with my setup-ruby fork, I just download the dist/index.js file and use it in setup-ruby-pkgs...

@eregon
Copy link
Member

eregon commented Mar 7, 2020

First of all, I hate repetitive or unneeded things.

Agreed. But I think the repetition is very reasonable here.

Both have a ruby-version: input.

I don't think setup-ruby-pkgs should have a ruby-version input.
I'm OK to export some extra environment variables like e.g. RUBY_PREFIX if it helps actions executed after.

So if I understand it's to allow this:

- uses: MSP-Greg/setup-ruby-pkgs@v1
  with:
    ruby-version: 2.6
    apt:
    brew:
    mingw:
    msys2:

instead of

- uses: ruby/setup-ruby@v1
  with:
    ruby-version: 2.6
- uses: MSP-Greg/setup-ruby-pkgs@v1
  with:
    apt:
    brew:
    mingw:
    msys2:

I don't think that's much repetition, and that hides which action does the setup for Ruby (I definitely don't wish for a third setup-ruby action, and so I think it should be clear which setup-ruby action is used).
GitHub Actions is more explicit than e.g. TravisCI which used a lot of magic. That implies it's a bit more verbose but it's also far clearer about what is happening.

So at this time I'm not convinced of the value of this.
Also, I wouldn't want to make all functions in this action public API, I want to be able to rename them at will, even if they are export-ed.

@eregon
Copy link
Member

eregon commented Mar 7, 2020

Another way to see it is it's basically "composition" by using 2 separate steps vs "inheritance" and mixing everything together, almost guaranteeing there will be some naming conflict or input/output/behavior mismatch if e.g. an input/output is added here.

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Mar 7, 2020

Sorry. I guess you like it or you don't.

IMHO, trying to compare a yaml API to other API's/structures is a stretch.

that hides which action does the setup for Ruby

Clear to me. Prior knowledge? Or, doesn't hide anything unless you're looking for 'setup-ruby'...

almost guaranteeing there will be some naming conflict or input/output/behavior mismatch if e.g. an input/output is added here.

'almost guaranteeing'? Doubt it. These aren't complicated.

@eregon
Copy link
Member

eregon commented Mar 7, 2020

IMHO, trying to compare a yaml API to other API's/structures is a stretch.

I meant all export-ed functions in the JS file become public API, instead of private API.

Clear to me.

Of course, but not so clear to others whether it's actions/setup-ruby, ruby/setup-ruby, or something else.

I don't see the need for this, can you do without? If there is a lot of feedback that users want that then let's reconsider.

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Mar 7, 2020

If there is a lot of feedback that users want that

Unlikely. For things like this, users take what you give them, they don't ask for API topology changes.

After all, no one asked for this action (setup-ruby), they asked for more platforms, current releases, and specific Ruby patch/teeny releases.

In general, the top level task is performing CI, and configuring the system is one of the early tasks. Allowing a package install action to also install Ruby just eliminates an extra workflow step.

If one wants to properly handle windows build tools, one still has to spin up the Ruby instance. So, the package action doesn't benefit from importing setup-ruby.

@MSP-Greg MSP-Greg closed this Mar 7, 2020
@eregon
Copy link
Member

eregon commented Mar 7, 2020

one still has to spin up the Ruby instance

As said earlier, that can be easily solved by setting an env var from this action e.g. RUBY_PREFIX and/or e.g. RUBY_NAME (the validated string given as the ruby-version input).

@MSP-Greg MSP-Greg deleted the external-use branch March 7, 2020 19:10
@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Mar 7, 2020

Re Windows, at present, most repos have been using the most recent patch/teeny releases of Rubies. Those releases were all compiled with the most recent MSYS2 gcc release.

As one moves to older ones, there may be gcc issues. Rather than embed the data for that in the action, I'd rather query the Ruby version in use, and decide what to do based on the gcc info embedded in RbConfig. Same may also be easiest for OpenSSL. I think...

Re spinning up Ruby in general, when that was included in the PR, the MRI builds did not seem to take appreciably longer for the action run step, so I'm not to concerned about the time.

Finally, I added code to download dist/index, and the required it. It runs fine, so export really isn't needed.

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Mar 7, 2020

Trying to format for the UI here, the code to use 'setup-ruby' is essentially:

const uri = 'https://raw.githubusercontent.com/ruby/setup-ruby/v1/dist/index.js'
const dl = await tc.downloadTool(uri)
await require(dl)

@reitzig
Copy link

reitzig commented May 29, 2020

I agree that not enabling this does not prevent it. If action authors want to spare their users the extra step(s), they will.

So why not give them a properly versioned (and potentially cached?) NPM package to work with?

@eregon
Copy link
Member

eregon commented May 29, 2020

@reitzig The run() function was exported in #42, and setup-ruby-pkgs uses it that way: https://github.com/MSP-Greg/setup-ruby-pkgs/blob/baf877d099a338bf29f15a238c47da27cf78f260/index.js#L35-L36

I have no plans to maintain an extra npm package, this is an action and only makes sense in the context of running in GitHub Actions. And it doesn't seem needed for other actions to use it.
Using the v1 link is "properly versioned", isn't it?

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented May 29, 2020

this is an action and only makes sense in the context of running in GitHub Actions

👍

Using the v1 link is "properly versioned", isn't it?

A long time ago, I thought an 'actions' recommendation was to have a vX branch track the most recent release, where X is the major version. Because of that, I've always done updates in master, when complete, rebased branch v1 on master, then created tags/releases on v1's head. In actions/cache, they're using 'releases/v1' instead.

Interesting question.

@reitzig

You have options as to how you handle action inputs. You can add all of the inputs for this action to your own, as I've done in set-ruby-pkgs, or you can hard code values in your js code. If you look at actions/core code, I believe the runner code uses the input name, parses it, and that string is used to set an ENV variable with the input's value.

Note that if an input is added to setup-ruby, your action will need to handle that. The runner code handles the default values from action.yml. It probably would have been better if a pattern was suggested where the action js code handled the defaults (which would require reading the yml file).

@eregon
Copy link
Member

eregon commented May 29, 2020

To clarify, if an npm package would help I might consider it, but I'd like to hear why it's better and how can it be automated so it's no extra effort compared to currently (just creating a release in GitHub UI, v1 is bumped automatically).

Regarding inputs, I could probably move the defaults inside the .js file, but it would introduce some duplication. Having a function taking direct arguments instead of core.getInput() could be nice, but then how to evolve it when there are new inputs?

I think right now it's best if actions reusing this action transparently pass all possible inputs, so the end user can configure as needed.

@reitzig
Copy link

reitzig commented Jun 2, 2020

And it doesn't seem needed for other actions to use it.

I don't know how the JS ecosphere usually works, but I would expect npm install to provide significant advantages over curl. Dependency tracking, automatic inclusion of bugfix upgrades, clarity of code, clearly defined API (@MSP-Greg mentions the things defined in action.yml which we circumvent by using any function directly), etc.

Using the v1 link is "properly versioned", isn't it?

Only if you never change that tag. I'm new to writing Github Actions, but I get the impression that people use Git tags more like Docker tags here, that is re-set v1 to the latest v1.x.y revision?

how can it be automated so it's no extra effort compared to currently (just creating a release in GitHub UI, v1 is bumped automatically).

I won't be of much help here myself (see above), but I imagine that once there's agreement on what to package/publish, doing so as part of CI/CD is the least worry. Pushing it to the GitHub Packages registry of this repo would be the natural choice, I think, which would also avoid the impression of a "general purpose" library.

I think right now it's best if actions reusing this action transparently pass all possible inputs, so the end user can configure as needed.

That's fair, I think. Make all parameters mandatory, implicitly. A separate entry function that takes some form of parameter object (I'd say "data class", but Javascript...) as opposed to calling run() with input living in some global space would be advisable in that case. The "action" function run() would then use core.getInput to assemble the action input and call that same function.

but then how to evolve it when there are new inputs?

As long as the calling action doesn't update their dependency, they don't have to change anything.
If and when they do, the same rules as for any library apply?

I acknowledge that this request does add the perspective of "library maintainer" to a project that, on first glance, doesn't need that. I would argue it's worth it for the ecosystem at large, though.


Seems the Python folks have a similar discussion going on, with a workaround that involves use of NPM at the consumer action: actions/setup-python#38

@eregon
Copy link
Member

eregon commented Jun 6, 2020

Dependency tracking

Already handled as it's a self-contained .js script with https://raw.githubusercontent.com/ruby/setup-ruby/v1/dist/index.js

automatic inclusion of bugfix upgrades

This will automatically work with the link. With a npm package, you would always have to update manually which seems cumbersome. If an update is missed, then your action might expose bugs that are already fixed upstream, which seems very suboptimal.

Only if you never change that tag. I'm new to writing Github Actions, but I get the impression that people use Git tags more like Docker tags here, that is re-set v1 to the latest v1.x.y revision?

v1 is a branch, not a tag, and it's updated on every release (so it means latest 1.x.y release).
Using older releases of ruby/setup-ruby is not recommended, see https://github.com/ruby/setup-ruby#versioning

A separate entry function that takes some form of parameter object (I'd say "data class", but Javascript...) as opposed to calling run() with input living in some global space would be advisable in that case. The "action" function run() would then use core.getInput to assemble the action input and call that same function.

Yeah, I think that's simple enough, I'll do that.

I won't be of much help here myself (see above), but I imagine that once there's agreement on what to package/publish, doing so as part of CI/CD is the least worry. Pushing it to the GitHub Packages registry of this repo would be the natural choice, I think, which would also avoid the impression of a "general purpose" library.
I acknowledge that this request does add the perspective of "library maintainer" to a project that, on first glance, doesn't need that. I would argue it's worth it for the ecosystem at large, though.

I don't know much about the process for that, and given I have rather little time for this action, I'd rather focus on the core action and what's strictly needed to use it in other actions.

eregon added a commit that referenced this pull request Jun 6, 2020
…ctions

* Inputs are passed as simple object properties.
* It is recommended to expose the same inputs for other actions.
* See #58 and #33.
@eregon
Copy link
Member

eregon commented Jun 6, 2020

I added a setupRuby as an exported function taking object properties for inputs: 81be093

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

Successfully merging this pull request may close these issues.

3 participants