-
Notifications
You must be signed in to change notification settings - Fork 42
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
Updated cookbook for habitat API post 0.56.0 #115
Updated cookbook for habitat API post 0.56.0 #115
Conversation
Added hab_user_toml resource Added qol features for private builder and private organizations Signed-off-by: W. Duncan Fraser <wdf@alasconnect.com>
Signed-off-by: W. Duncan Fraser <wdf@alasconnect.com>
I tried if this PR would solve my issues regarding installing private packages, however it still fails when trying to install a private package. I think you missed to fix one spot, where it currently is tried to determine the availability of the package via an http request. You can find my PR to fix this here. Otherwise I would love to see something like this get merged, however it probably would have been easier to follow along the changes if it would have been multiple PRs. I just hope you know how to get PRs merged within this repo. I was trying to get some feedback from the maintainers for a few weeks on the way the unsupported hab token should be implemented, but did not have any luck at all (I mostly got ignored when following official recommendations). I think we could do properties like you did, use the env var (as this is what hab usually does), or use a cookbook attribute (which would be neat as well, as you only have to specify it once) However, last time someone needed to get a PR merged, they pinged @tas50 . Let's see how that goes this time... |
@st-h I'll re-validate the stand alone package installation on my end and get that fix pulled in 👍 . Did you also attempt via service load, to where the supervisor itself handles the installation? That I validated thoroughly across the three service providers on our end as we use that workflow internally. I'll see if I can poke anyone on the habitat/chef side and may break this up into multiple PRs if the maintainers would prefer. I was largely just porting work over from updating the cookbook for internal use and wanted to start the conversation. |
Signed-off-by: W. Duncan Fraser <wdf@alasconnect.com>
@wduncanfraser Yeah, iirc the service resource only seems to need the environment set on the init/upstart etc scripts. Otherwise setting the env var works well, except for the http request that bypasses the hab client, that causes the install resource to fail. That actually took me quite some time to figure out, as I wasn't expecting a wild http request to suddenly appear at all. Let's hope we will get some feedback soon. I am running a patched version myself, but did not submit everything yet, I was hoping to get a response from the maintainers myself before doing so. |
btw, this PR should fix #114 as well |
I think we could also get rid of that http request, if we could use the |
Yeah, it would probably be good to depend on the hab search API in the future, as then it's less that has to be maintained in the cookbook itself. Though I will leave that task for a separate PR :) |
@wduncanfraser absolutely 💯 I don't know how functional the search api actually currently is though - maybe that's why they used a plain http request in first place. |
@st-h Just wanted to check in and make sure everything was working for you. Sounds like the primary maintainer for the cookbook is out of the office ATM which is why it has been quite on the PR. |
@wduncanfraser just was about to comment 😀 I deployed your branch to prod this week as this is the most reliable version of this cookbook I have seen so far. I was having some issues with updating to a new version of a package that was installed via the hab_package resource. The new version did not get picked up on a chef client run. Did not have time to look closer into this though. who is the primary maintainer? |
@st-h That would be @jtimberman to my knowledge. |
I'll be taking a look at this PR over the next day or two and hopefully cutting a new release soon :) |
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 added one request and a comment to this PR.
Since this PR is adding a lot of new things it would be great if it could be split up into a few smaller PRs as mentioned earlier in the conversation, specifically the parts which add / change resources, since it will make it a lot easier to review. If you'd be OK with doing that, my aim is to have this reviewed and a new release cut in the next couple of days - at first blush though, this looks good.
end | ||
end | ||
|
||
action_class do | ||
HAB_VERSION = '0.59.0'.freeze |
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.
This version pin replicates the pin defined in https://github.com/chef-cookbooks/habitat/blob/master/resources/install.rb#L86. If we're going to be pinning to that version in multiple places, I'd like us to extract that somewhere to be shared between all locations which need to pin.
@@ -116,7 +122,7 @@ def depot_package(name, version = nil) | |||
name_version = [pkg_name, version].compact.join('/').squeeze('/').chomp('/').sub(%r{^\/}, '') | |||
url = "#{new_resource.bldr_url.chomp('/')}/v1/depot/channels/#{origin}/#{new_resource.channel}/pkgs/#{name_version}" | |||
url << '/latest' unless name_version.count('/') >= 2 | |||
Chef::JSONCompat.parse(http.get(url)) | |||
Chef::JSONCompat.parse(http.get(url, Authorization: "Bearer #{new_resource.auth_token}")) |
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.
What is the behaviour here where no auth_token has been specified?
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'll add a conditional on this.
@jonlives I'll work on getting this split up into smaller PRs today. |
Awesome thank you! |
@wduncanfraser do you think you'll be able to get around to the smaller PRs? If time is a blocker I can go through the single larger PR. |
@jonlives I apologize for the delay, got a little crazy on this end. Working on this now. |
@jonlives Opened up new PRs breaking out changes. |
Awesome thank you for getting to those, will go through them and try and get a new release cut as soon as I can. |
Closing this PR in favour of the new sub-PRs containing the same changes |
Description
Issues Resolved
This closes #106
This closes #102
Check List