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

Only bundle install when a Gemfile.lock exists #396

Merged
merged 1 commit into from
Sep 30, 2020
Merged

Conversation

huwd
Copy link
Member

@huwd huwd commented Sep 9, 2020

A response to issue #389, govuk-docker contains some gems that are
needed to use with other applications that tend not to have a
Gemfile.lock when initialy cloned. This was causing make (eg. make gds-api-adapters) to fail.

Here we make the depdency explicit and skip the step if no Gemfile.lock
exists, a step that hasn't proved disruptive to using the adapters in
other applications.

A response to issue #389, govuk-docker contains some gems that are
needed to use with other applications that tend not to have a
Gemfile.lock when initialy cloned. This was causing make (eg. `make
gds-api-adapters`) to fail.

Here we make the depdency explicit and skip the step if no Gemfile.lock
exists, a step that hasn't proved disruptive to using the adapters in
other applications.
@huwd huwd linked an issue Sep 9, 2020 that may be closed by this pull request
@@ -38,7 +38,7 @@ all-apps: $(APPS)
bundle-%: clone-%
$(GOVUK_DOCKER) build $*-lite
$(GOVUK_DOCKER) run $*-lite rbenv install -s || ($(GOVUK_DOCKER) build --no-cache $*-lite; $(GOVUK_DOCKER) run $*-lite rbenv install -s)
$(GOVUK_DOCKER) run $*-lite sh -c 'gem install --conservative --no-document bundler -v $$(grep -A1 "BUNDLED WITH" Gemfile.lock | tail -1)'
if [ -f Gemfile.lock ]; then $(GOVUK_DOCKER) run $*-lite sh -c 'gem install --conservative --no-document bundler -v $$(grep -A1 "BUNDLED WITH" Gemfile.lock | tail -1)'; fi
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we haven't installed bundler for a gem?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting, I'd not considered that.
I'm going to go through the rest of the repos that have a gemspec and see what the state of play is. Then I'll feed back here if there are any that don't have bundler installed. I should be able to see if this step trips up any others we try to make.

If there are not, then perhaps we can have a chat about best pratice and what's good enough for now?

If there are some, then perhaps I'll take this line and abstract it off into it's own make step or bash function that can choose if we install bundler with slightly more complex logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, thanks. I only recently learnt that Ruby now comes with bundler pre-installed, but it would be good to verify all the gemspec projects we support actually use a sufficient version for that. If not, they could just have their own custom Ruby/bundle thing in their Makefile, since they're intent on being special (or we could just upgrade Ruby!).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah interesting!
Looks like bundler has been in standard lib since ruby 2.6.0.
I'll take a quick look now for anything we've got in GOV.UK docker with a gemspec, and see if it's >2.6.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

so I think i've found five gems that are supported by govuk-docker.
From my ~/govuk if i run:

cat govspeak/.ruby-version && \
cat plek/.ruby-version && \
cat gds-api-adapters/.ruby-version && \
cat govuk_publishing_components/.ruby-version && \
cat govuk_app_config/.ruby-version

i get:

2.6.3
2.6.6
2.6.6
2.6.6
2.6.5

So currently I believe all supported gems are a version above 2.6.x and so should come with bundler in standard lib! 🎉

Copy link
Member Author

Choose a reason for hiding this comment

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

So for the moment you happy to approve with this if statement?
If we have a need for more gems in future, or to support earlier ruby versions I'm happy to look at something more elegant?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks for investigating! I'm glad we can keep it simple.

@huwd huwd mentioned this pull request Sep 10, 2020
Copy link
Contributor

@benthorner benthorner left a comment

Choose a reason for hiding this comment

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

🎉

@@ -38,7 +38,7 @@ all-apps: $(APPS)
bundle-%: clone-%
$(GOVUK_DOCKER) build $*-lite
$(GOVUK_DOCKER) run $*-lite rbenv install -s || ($(GOVUK_DOCKER) build --no-cache $*-lite; $(GOVUK_DOCKER) run $*-lite rbenv install -s)
$(GOVUK_DOCKER) run $*-lite sh -c 'gem install --conservative --no-document bundler -v $$(grep -A1 "BUNDLED WITH" Gemfile.lock | tail -1)'
if [ -f Gemfile.lock ]; then $(GOVUK_DOCKER) run $*-lite sh -c 'gem install --conservative --no-document bundler -v $$(grep -A1 "BUNDLED WITH" Gemfile.lock | tail -1)'; fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks for investigating! I'm glad we can keep it simple.

@huwd huwd merged commit f0cb41e into master Sep 30, 2020
@huwd huwd deleted the fix-gds-api-adapters branch September 30, 2020 09:29
huwd added a commit that referenced this pull request Sep 30, 2020
In commit #396 I introduced a mistake. The logic looks for a gemfile, but I think it's looking in the wrong directory, being explicit about where it should look helps with individual builds
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.

GDS API Adapters fails to build
2 participants