Memoize Hatchet::App.default_buildpack

[edmorley]

To improve performance and so that it still returns the correct result if called again inside an app.deploy block.

Fixes #180.

[edmorley]

@schneems Actually, it seems that HATCHET_BUILDPACK_BASE and HATCHET_BUILDPACK_BRANCH are themselves effectively memoized due to them using the = -> { ... } syntax here?

hatchet/lib/hatchet/app.rb

Lines 8 to 14 in a245b8c

	HATCHET_BUILDPACK_BASE = -> {
	ENV.fetch('HATCHET_BUILDPACK_BASE') {
	warn "ENV HATCHET_BUILDPACK_BASE is not set. It currently defaults to the ruby buildpack. In the future this env var will be required"
	"https://github.com/heroku/heroku-buildpack-ruby.git"
	}
	}
	HATCHET_BUILDPACK_BRANCH = -> { ENV['HATCHET_BUILDPACK_BRANCH'] || ENV['HEROKU_TEST_RUN_BRANCH'] || Hatchet.git_branch }

I wasn't familiar with the "stabby lamda" operator and had missed the =, so had initially read those lines as being computed each time.

If that's the case, then I don't think this PR is necessary, and actually the issue in #180 was that I should have been using app.class.default_buildpack and not Hatchet::App.default_buildpack, since the latter creates a new app instance so recomputes HATCHET_BUILDPACK_BRANCH from a different working directory.

[schneems]

seems that HATCHET_BUILDPACK_BASE and HATCHET_BUILDPACK_BRANCH are themselves effectively memoized due to them using the = -> { ... } syntax here?

This is a stabby lambda being set to a constant. Every time it is called it will be re-evlauated.

ENV["HATCHET_BUILDPACK_BRANCH"] = "foo"
 HATCHET_BUILDPACK_BRANCH = -> { ENV['HATCHET_BUILDPACK_BRANCH'] || ENV['HEROKU_TEST_RUN_BRANCH'] || Hatchet.git_branch } 
puts HATCHET_BUILDPACK_BRANCH.call
# => foo

ENV["HATCHET_BUILDPACK_BRANCH"] = "bar"
puts HATCHET_BUILDPACK_BRANCH.call
# => bar

The reason it's done like this instead of simply setting a constant is that someone might set this env var after the library is loaded, so we need it to re-evaluate the contents at runtime (instead of require time).

[schneems]

LGTM. My only question would be "Does this need a test to assert memorizing the value?"

expect(Hatchet::App.default_buildpack.object_id).to eq(Hatchet::App.default_buildpack.object_id)

This asserts the object is not just equal, but that it's the same object.

I'm on the fence. I think this PR is probably fine without that assertion if you want to go ahead and merge. Otherwise adding a test and merge would be good.

Thanks for working on this!

[edmorley]

Oh sorry I follow now, the constants are being assigned an anonymous function, not a value, hence the need for .call. I'm used to that construct in Python/JS but for some reason that syntax threw me off.

I'm happy to add a test, I'll do that now.

[schneems]

When you write a test please also rebase. Changelogs are merge conflict tar-pits. Let me know if you've got any tricks up your open source sleeves for better handling them.

[edmorley]

Changelogs are merge conflict tar-pits. Let me know if you've got any tricks up your open source sleeves for better handling them.

I don't think there are many good solutions if we want to keep the changelog up to date in-between releases (which I think we do). I tend to just resolve as I go, post-review but pre-merge. Thankfully doesn't happen too often, between PRs normally being spaced out in time, and then a proportion of them being [skip changelog].

[schneems]

Thanks!