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

Habitat provisioner updates #22705

Merged
merged 5 commits into from
Oct 1, 2019
Merged

Habitat provisioner updates #22705

merged 5 commits into from
Oct 1, 2019

Conversation

kmott
Copy link
Contributor

@kmott kmott commented Sep 5, 2019

This is a continuation of: #21776

This PR should include most all feedback from the original PR, and gets tests running on master again.

[ ... ]

=== RUN   TestLinuxProvisioner_linuxInstallHabitat
--- PASS: TestLinuxProvisioner_linuxInstallHabitat (0.00s)
=== RUN   TestLinuxProvisioner_linuxStartHabitat
--- PASS: TestLinuxProvisioner_linuxStartHabitat (0.00s)
=== RUN   TestLinuxProvisioner_linuxUploadRingKey
--- PASS: TestLinuxProvisioner_linuxUploadRingKey (0.00s)
=== RUN   TestLinuxProvisioner_linuxStartHabitatService
--- PASS: TestLinuxProvisioner_linuxStartHabitatService (0.00s)
=== RUN   TestResourceProvisioner_impl
--- PASS: TestResourceProvisioner_impl (0.00s)
=== RUN   TestProvisioner
--- PASS: TestProvisioner (0.00s)
=== RUN   TestResourceProvisioner_Validate_good
--- PASS: TestResourceProvisioner_Validate_good (0.00s)
=== RUN   TestResourceProvisioner_Validate_bad
--- PASS: TestResourceProvisioner_Validate_bad (0.00s)
=== RUN   TestResourceProvisioner_Validate_bad_service_config
--- PASS: TestResourceProvisioner_Validate_bad_service_config (0.00s)
=== RUN   TestResourceProvisioner_Validate_bad_service_definition
--- PASS: TestResourceProvisioner_Validate_bad_service_definition (0.00s)
PASS

Process finished with exit code 0

Note that the Chef license changes are still a string to be more explicit about how the license acceptance works on the system you are provisioning Chef Habitat to.

Closes #20684
Closes #19394
Related to #18492
Closes #17799

@ooOOJavaOOoo
Copy link

Not 100% sure why this failed, but I'm willing to help get this going if needed

@kmott
Copy link
Contributor Author

kmott commented Sep 6, 2019

I'm not sure either, but I think git.apache.org may be down (which fresh builds have a dependency on): https://status.apache.org

@kmott
Copy link
Contributor Author

kmott commented Sep 11, 2019

@pselle I will work on getting my changes merged with the changes from #22745...

@kmott
Copy link
Contributor Author

kmott commented Sep 12, 2019

@pselle, I reviewed @apparentlymart provisioner changes from e0d7293#diff-b19db9165cff9397acb294490cf66308, and I was wondering if I should close this PR? It sounds like the recommendation is to not use provisioners moving forward, so I should probably not put these changes in (as that would go against the new policy)?

@pselle
Copy link
Contributor

pselle commented Sep 17, 2019

@kmott I think that's a fair read -- adding more features to provisioners sends a mixed message when we're telling people "please don't use these", and that closing this PR is a reasonable action in that context.

@kmott
Copy link
Contributor Author

kmott commented Sep 17, 2019

@pselle I guess my concern with that approach is there's a lot of Habitat (and I'm sure other provisioners) that needs to be setup when an instance is created (ring data, package installation, etc.), but you can't/shouldn't do that during the build/package phase with something like Packer (since it will be unique to each instance/deployment).

If that first-class support for the provisioner isn't in-place in Terraform, then it's left to a bunch of remote_exec's and file() calls, which (as we all know) is cumbersome and error prone (they can be abstracted away to modules to reduce some of that complexity/duplicity, but it's still a concern, at least in my mind).

@pselle
Copy link
Contributor

pselle commented Sep 24, 2019

@kmott I've gotten some clarification -- while the stance on provisioners reads strong, provisioners do exist currently in Terraform and basic maintenance to keep them working is the plan until/if an explicit plan on phasing out provisioners comes to fruition. To say it another way, the stance is not meant as "never use provisioners" but rather "consider these other options instead and prefer them whenever possible."

As to your point about requirements of Habitat's architecture, there are possibly ways to achieve the goals of connecting to a ring with user_data (as the provisioners documentation notes for some resources with specifics https://www.terraform.io/docs/provisioners/index.html#passing-data-into-virtual-machines-and-other-compute-resources). I'm sure you have more Habitat experience than I do, so if/once you discover these paths, we'd be glad to include them in the docs so we can start pointing Habitat users toward helpful routes.

Now as to this PR specifically then -- I'm happy to move this forward (once de-conflicted) with the thought in mind of: while we can discourage users from using provisioners, they are still in Terraform, and your changes include bugfixes that would benefit such users (including those who filed the tickets you linked to, and surely more).

I hope this response helps some?

@kmott
Copy link
Contributor Author

kmott commented Sep 24, 2019

Yeah, thank you @pselle, I will get on this as soon as I can. :-)

…cense`, adjust how license acceptance is done, update hab provisioner doc.
@kmott
Copy link
Contributor Author

kmott commented Sep 29, 2019

@pselle, I think my last commit is the final bit of cleanup needed. Hopefully I did the merge and conflict resolution correctly (git still confuses me sometimes, ;-)).

I did test on a local vSphere deployment using hab 0.79.1 and 0.85.0 with several permanent peers, several machines with other services and binds, and it all seemed to work together just fine.

Let me know if I need to adjust anything else. Thanks again for all of your help in brokering this ticket! :-)

@pselle pselle self-requested a review September 30, 2019 13:51
Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

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

One question/thing that might need a fix, but otherwise lgtm!

@ghost
Copy link

ghost commented Nov 1, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Nov 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants