-
Notifications
You must be signed in to change notification settings - Fork 680
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
Improvements to Habitat plan #1820
Conversation
Can't currently verify due to ongoing public Depot issues. Will revisit this later. This seems pretty good to me at first glance though. |
habitat/plan.sh
Outdated
} | ||
|
||
do_prepare() { | ||
# Create a Gemfile with what we need | ||
cat > Gemfile <<GEMFILE | ||
cat > "$CACHE_PATH/Gemfile" <<GEMFILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've always been confused by this step. We have a gemfile in the main repo, why recreate one here? I think this means we will always be building the version in rubygems and never in master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's intended currently. Ultimately I'd rather this be part of a CI pipeline that cut a release anytime we promoted to rubygems, but seeing as how these tasks can sometimes be async, this feels like the safest way for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elliott-davis the main reason for this is that there is now way to bundle install without having bundler write out a .bundle/config to the same directory where the Gemfile is located, so if you build using the "real" Gemfile, then you won't be able to bundle anything on your local machine, because bundler will save those settings.
I'm pushing up a commit where we use path: $SRC_PATH
instead of = $pkg_version
. This will make it use the current version on disk rather than pulling from rubygems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in d030d74.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @smith!
Going to hold on merging this until after 1.25 is released, will get this in next week's 1.26 release. |
} | ||
|
||
do_prepare() { | ||
# Create a Gemfile with what we need | ||
cat > Gemfile <<GEMFILE | ||
# If we use the Gemfile in the project ($SRC_PATH/Gemfile), and run `bundle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is avoidable: https://github.com/habitat-sh/core-plans/blob/master/scaffolding-ruby/lib/scaffolding.sh#L162-L192 but it's fine for this. It'd be cool to get the ruby scaffolding working for ruby bins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that's another solution, but I wouldn't necessarily call it a better one :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @smith for this improvement
These are kind of all over the place, but should improve things: * Use the new `pkg_version` mechanism to set the version, and fail if the VERSION file is not present * Use inspec.io for the upstream url * Remove pkg_source and it's associated callbacks; they aren't required any more * Alphabetize the deps list * Remove duplicate coreutils from build deps * Move environment variable setting to `do_prepare` * Delete all binstubs in bin that aren't inspec * Put the generated Gemfile in $CACHE_PATH so it doesn't stomp on the developer's Gemfile * Insert the SSL_CERT_FILE env var in the binstub (Fixes #1582) * Use install instead of cp to drop off Gemfile.lock * Build using `path: '$SRC_PATH'` instead of `'= $pkg_version'` in the Gemfile * Disable `do_strip` to decrease build time and because we don't need it Works for me on Habitat 0.23. Since all the "building" is done now in `do_install`, it would be possible to define a `do_check` that runs `inspec exec` on profiles to verify inspec is working by running inspec. Signed-off-by: Nathan L Smith <smith@chef.io>
d030d74
to
6324a6d
Compare
In #1820 we made it so inspec would install the checked out source version rather than the version from RubyGems. This actually didn't work (though it wasn't apparent in a development environment) because it used a relative path to bin/inspec that pointed at /src/bin/inspec, which only exists if you're in a Habitat studio started from the InSpec repo. Revert back to getting the gem from RubyGems to avoid this problem and have a working package. Signed-off-by: Nathan L Smith <smith@chef.io>
In inspec#1820 we made it so inspec would install the checked out source version rather than the version from RubyGems. This actually didn't work (though it wasn't apparent in a development environment) because it used a relative path to bin/inspec that pointed at /src/bin/inspec, which only exists if you're in a Habitat studio started from the InSpec repo. Revert back to getting the gem from RubyGems to avoid this problem and have a working package. Signed-off-by: Nathan L Smith <smith@chef.io>
These are kind of all over the place, but should improve things:
pkg_version
mechanism to set the version, and fail ifthe VERSION file is not present
any more
do_prepare
developer's Gemfile
Works for me on Habitat 0.23.
Since all the "building" is done now in
do_install
, it would bepossible to define a
do_check
that runsinspec exec
on profiles toverify inspec is working by running inspec.
Signed-off-by: Nathan L Smith smith@chef.io