Allow override of bundler version or resolve from lock file

helaili / jekyll-action

A GitHub Action to publish Jekyll based content as a GitHub Pages site

MIT License

250 stars 120 forks source link

Allow override of bundler version or resolve from lock file #114

Closed inverse closed 2 years ago

inverse commented 2 years ago

Allow override of the default provided bundler from the image or resolve from Gemfile.lock if one exists.

inverse commented 2 years ago

@helaili anything you would change on this?

inverse commented 2 years ago

@helaili any other feedback?

helaili commented 2 years ago

Sorry @inverse, time is a scarce resource. Would you mind adding a test case similar to this one?

inverse commented 2 years ago

@helaili I added two cases cases where it would do what you outlined - I think it's correct?

I also added asserting the bundler version matches either explicitly or from the lockfile.

helaili commented 2 years ago

Tests are still failing. Trying to debug it, I've seen this line.

Thing is bundler is being installed within the container, and the echo "::set-output name=value::$(bundler -v | cut -c 16- | xargs)" command is being called within the next step, so outside of the container where bundler is not installed or could be a totally different one.

You need to run the set-output command from entrypoint.sh and referred to the value as coming from the run step. This step should actually be renamed for clarity.

inverse commented 2 years ago

Ahh you're totally right :see_no_evil:

You need to run the set-output command from entrypoint.sh and referred to the value as coming from the run step. This step should actually be renamed for clarity.

What do you mean by that? I don't follow?

I pushed a change which instead does the following:

Adds the bundler version to site config and exposes it on the page
Modifies the cypress tests for the two additional flows to assert the bundler version

inverse commented 2 years ago

@helaili good catch! how does the graph look now? https://github.com/helaili/jekyll-action/actions/runs/2296309955

helaili commented 2 years ago

The graph is good now but the test is still failing works (failed)

inverse commented 2 years ago

@helaili I think I know why...

So this test case is when we force install a newer version of bundler than what the application is bundled with. Yet the system has 2.1.4 so it bundles with that version as it's in the lockfile :/

Not sure what a good test would be for this case? We would need to some how remove the system bundler version before for that test?

I could remove that part and we just install the bundler version from the lockfile which probably covers the majority of cases...

helaili commented 2 years ago

I'm removing the Gemfile.lock file during the jekyll_bundler_run job so that it is generated with the right Bundler version. I'm also now relying on the bundle -v command as opposed to bundler -v to figure out the version number as `bundler is not in the path in this specific case.

Thanks for this contribution @inverse

Tested in #132

inverse commented 2 years ago

@helaili That's for taking the time to explain :+1: