Skip to content
This repository has been archived by the owner on Feb 9, 2021. It is now read-only.

Provide a link for users wanting exact or more versions #52

Closed
wants to merge 1 commit into from

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Jan 17, 2020

Original PR title:

Clarify the supported versions and intended usage

README.md Outdated Show resolved Hide resolved
@eregon
Copy link
Contributor Author

eregon commented Jan 17, 2020

@bryanmacfarlane Could you review?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@bryanmacfarlane
Copy link
Member

I'm going to propose some alternative text - busy in a few meetings so I should be able to follow up later today or the weekend.

@eregon
Copy link
Contributor Author

eregon commented Jan 17, 2020

I just pushed a commit to simplify and address your comments.

Could you maybe merge this PR and then you could refine the text later? (it's likely also more efficient)
I think it's important to have this clarification soon in the README.

@bryanmacfarlane
Copy link
Member

bryanmacfarlane commented Jan 17, 2020

I think it's important to have this clarification soon in the README.

Agreed! I pushed a simpler statement in the meantime. A simpler statement is better without confusing folks about details of the cache, specific version links (which puts you down a wrong road) and then clarifying details around that.

I updated readme to simply point out to bind to minor versions and which minor versions are everywhere.

https://github.com/actions/setup-ruby/blob/master/README.md#setup-ruby

@bryanmacfarlane
Copy link
Member

I also don't want to point to the prebuilt Ruby work until we decide on the plan and if / when we're going to fund it.

@eregon
Copy link
Contributor Author

eregon commented Jan 17, 2020

That's not very nice, I spent some time to address your feedback here and basically you push something that would conflict hopelessly with this PR. Moving on,

I also don't want to point to the prebuilt Ruby work until we decide on the plan and if / when we're going to fund it.

I don't understand that, it's just the mention of a plan and #44 (comment) makes it clear it's a wanted direction, no?
Anyway, I dropped that sentence and rebased on top of master.

Could you merge the added paragraph in this updated PR?

I would think most Ruby users in Actions will want exact versions or more versions, so it seems very useful to have a link for a way to achieve that.
Especially since as you mention it might take a while until this action supports that.

@bryanmacfarlane
Copy link
Member

bryanmacfarlane commented Jan 17, 2020

@eregon - apologies. I thought we both agreed that it was critical that we get the point about only binding to minor versions in the README asap while we iterate on the other comments. Before your recent force push, there was quite a bit of of information and suggestions we were iterating on. So I pushed the one sentence we agreed was time critical.

With the latest force push, the topic seems to have changed to recommending another marketplace action from the readme of this action. I'm following up with product but I believe that's the role of the marketplace fly out (type ruby in that and yours comes up)

setup-ruby

@eregon
Copy link
Contributor Author

eregon commented Jan 17, 2020

The diff before the force push (no choice since there was a conflict) is here: master...eregon:patch-2-original. I thought from your commit to master you wanted something significantly simpler, which seems good indeed so I dropped the extra details.

I'm following up with product but I believe that's the role of the marketplace fly out (type ruby in that and yours comes up)

In #44 (comment) it is clear this action aims to be "what the community wants".
What the community wants is clearly more versions and the ability to pinpoint a fixed/exact version.

I think adding a sentence in the README about this is a great way to achieve that, until this action itself supports that. I don't think people have the reflex to go to the Marketplace when there is an official action for it, so this would help them get what they want to do with GitHub Actions.

Another point of view is this action currently fits a set of use cases but also does not meet other use cases (more and exact versions) that would be expected by many for "setting up Ruby". I believe having a mention and link in the README for other use cases and acknowledging them seems a lot more welcoming for those users than nothing about it.

@MSP-Greg
Copy link
Contributor

  1. All OS's seem to have 2.7.0 in the toolcache. Might we add that if any more pushes/commits are done?

  2. Maybe in the future when new releases are added to the toolcache, the previous version could be left on the image? Hence, there would always be two versions for a given major/minor release? Some repo's prefer to fix the teeny version, and only change it in the workflow yaml after verifying that CI passes...

  3. Various Ruby version managers and also other CI has supported a syntax like '2.6', vs '2.6.x'. I ran this action with the '2.6' style, and it worked fine. Maybe start using that format in the README?

@bryanmacfarlane
Copy link
Member

Thanks @MSP-Greg - yeah I think there's many separate issues and topics here so it may be best to just create separate PR for each :).

  1. list 2.7.x - agreed, but I was going to validate it truly was in every pool and VM before adding it. I think it just finished but I want to confirm. Feel free to create a PR for that.
  2. Agreed on leaving the old version on the image. issue already here for compute team
  3. Minor version in the readme - yep, a PR was merged so the README samples show real minor version binding: https://github.com/actions/setup-ruby#usage
  4. version/versions typo: let's take
  5. Recommending particular marketplace actions. I have questions out to product (@chrispat @ethomson ) but I don't think we want that pattern.

For #1, I think the workflow in this repo should validate it using ./ (see checkout action for that) so we'll see the badge go red. That would run a full OS matrix. I can do that or a PR is fine.

@eregon
Copy link
Contributor Author

eregon commented Jan 17, 2020

If it's about wanting to avoid linking to a specific action, it could be a link to https://github.com/marketplace?utf8=%E2%9C%93&type=actions&query=Setup+Ruby
That gives a reasonable amount of actions.

https://github.com/marketplace?utf8=%E2%9C%93&type=actions&query=Ruby OTOH has a lot of actions unrelated to setting a Ruby interpreter, so that's going be rather unhelpful (even more so in the future with more actions).

I still think a direct link to https://github.com/eregon/use-ruby-action would be more useful for people reading this action's README and wanting more/exact versions. AFAIK none of the other setup ruby actions is nearly as complete.

@MSP-Greg
Copy link
Contributor

  1. list 2.7.x - agreed, but I was going to validate it truly was in every pool and VM before adding it. I think it just finished but I want to confirm. Feel free to create a PR for that.

See https://github.com/MSP-Greg/github-actions-ruby-info/actions. I checked before mentioning it...

  1. Agreed on leaving the old version on the image. issue already here for compute team

I added actions/runner-images#281 (comment). Not sure if it was clear...

  1. Recommending particular marketplace actions. I have questions out to product (@chrispat @ethomson ) but I don't think we want that pattern.

Maybe not long term, but until a decision is made on how to move forward, I think the Ruby community would benefit from its inclusion/mention. Could end up with a lot of repetitive issues without it...

@bryanmacfarlane
Copy link
Member

@MSP-Greg - thanks. Yep. I added a workflow in this repo so it's always validating all versions.
https://github.com/actions/setup-ruby/commit/dfc96c5570076adaa5950eecc3196b982053a06d/checks?check_suite_id=407123492

@eregon - thanks. let me chat with product. @ethomson @chrispat

@bryanmacfarlane
Copy link
Member

☝️ in the meantime, I was going to update readme to 2.7 after dinner unless someone beats me to it.

@MSP-Greg
Copy link
Contributor

I can update it. Ok to switch to 2.6 instead of 2.6.x? I'll mention that they're equivalent...

@bryanmacfarlane
Copy link
Member

... the workflow should confirm they're equivalent 😄

@MSP-Greg
Copy link
Contributor

See #54

@eregon eregon changed the title Clarify the supported versions and intended usage Provide a link for users wanting exact or more versions Jan 18, 2020
@eregon
Copy link
Contributor Author

eregon commented Jan 18, 2020

I rebased on top of the latest changes and adjusted the PR title.

CvX added a commit to discourse/discourse that referenced this pull request Jan 20, 2020
@bryanmacfarlane
Copy link
Member

@MSP-Greg I merged your PR ☝️ - thanks for that.

Regarding links to other market place actions on the readme main page, I spoke to product ( @ethomson and @chrispat ).

As pointed out above, there is a built in experience to discover other actions from the marketplace (below). We do not want start a pattern of adding links or recommending specific market place actions. We also don't want to add a link to every repo - that role has a built in experience.

If the demand for specific versions becomes an issue, It will push the priority of #49 ( note that we're discussing that right now as well ).

In addition, the actions/setup-* actions distribute binaries are either built and distributed by GitHub (with the chain secured) or pulled from an official or organization distribution channel (e.g. setup-node pulls from the official node distribution) in coordination with the virtual-environments team that manages that. Meeting with them today.

This discussion has taken on many many different topics and changes so we should close this out.

We ❤️ the marketplace and the options for users to discover other great actions and tools like TruffleRuby and others.

setup-ruby

@ethomson
Copy link
Contributor

We ❤️ the marketplace and the options for users to discover other great actions and tools like TruffleRuby and others.

I agree - thanks, @bryanmacfarlane for summarizing it like this. Overall, I think that giving actions visibility through the marketplace -- at the top level of the marketplace.

@eregon
Copy link
Contributor Author

eregon commented Jan 23, 2020

What about the idea of linking to a search such as this on the marketplace then?
I mentioned that idea above: #52 (comment)

The order on https://github.com/marketplace?utf8=%E2%9C%93&type=actions&query=Ruby seems rather random to me, although it's always the same. It's not ordered by stars, that's clear.
What is it ordered by?

The current marketplace gives me no confidence that a user will be able to find relevant actions, when e.g. the number of Ruby actions growth to multiple dozen actions. Even now it's already hard to find.

* Many Ruby users will want something like that as the issues on this
  repository make very clear.
* Once actions#44 is addressed
  we can remove this.
@eregon
Copy link
Contributor Author

eregon commented Jan 23, 2020

@ethomson

Overall, I think that giving actions visibility through the marketplace -- at the top level of the marketplace.

I'm not sure what you meant, maybe a few words are missing?

@bryanmacfarlane @ethomson I updated the PR with a link to the marketplace.

It looks like this:

If you want to use exact versions (e.g., 2.6.5) of Ruby, or to use JRuby or TruffleRuby,
please take a look at other actions on the marketplace.

I think such a general statement is going to be useful for users for a while, until this action supports all those versions.

If you still disagree, then close this PR.

If the demand for specific versions becomes an issue, It will push the priority of #49 ( note that we're discussing that right now as well ).

The demand has been clear since at least September and a very large number of users have been disappointed by setup-ruby (too few too old Ruby versions, unmaintained back then).
I would think those users either gave up on GitHub Actions, or had to use Docker images (which makes it impossible to test non-Linux platforms).

Is acknowledging in the README that there are other complementary "Setup Ruby" actions so hard?

Yes, this can be removed when this action supports as many versions as use-ruby-action. I'm not sure if (e.g. maybe this action will not want to support -head versions) and when that will happen though.

I made a proposal to get this done in a something like a week in #48. You want to make this your own way, that's all fine. But it makes me think it might take a couple months. And until then Ruby users are directed by GitHub to this action, which provides only 4 Ruby versions and no way to test against a specific version.

@bryanmacfarlane
Copy link
Member

As we pointed out above the problem of discovering alternative actions is the role of the marketplace

You want to make this your own way, that's all fine

This is not about one way or the other. This is about clear criteria.

We stated:

  • We will not give preference to one set of actions over others in the marketplace.
  • The role of discovering alternative actions is the role of the marketplace flyout. We are not going to start a pattern of adding recommendations to all of our actions (there are alternative implementations to most actions and that's a good thing).
  • There is a real binary distribution trust issue. We are not going to encourage users to acquire binaries into their automation distributed by a single user.

@actions actions locked as resolved and limited conversation to collaborators Jan 23, 2020
@bryanmacfarlane
Copy link
Member

The discussion in the PR has taken on so many topics and issues from the marketplace, distributions, minor version binding etc..

Thank you for the very active discussion.

☝️ Summary is above. If there are further questions, issues or questions, please open topic scoped issues. For the capability to consume specific versions, the discussion should be in ADR #49

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

Successfully merging this pull request may close these issues.

5 participants