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

[Provider] Rancher #9173

Merged
merged 2 commits into from
Dec 5, 2016
Merged

Conversation

johnrengelman
Copy link
Contributor

@johnrengelman johnrengelman commented Oct 3, 2016

This integrates the rancher provider from https://github.com/platanus/terraform-provider-rancher.
I've tried to retain the commit history for the interesting parts here in order to give credit to the original authors.

/cc @raphink @blackjid

@johnrengelman
Copy link
Contributor Author

@stack72 how do we go about providing the external system for the acceptance tests? Is there some scripting that we can add stuff to spin up docker containers or something?

@raphink
Copy link
Contributor

raphink commented Oct 3, 2016

@johnrengelman did you also take note of rancher/go-rancher#121 ?

@johnrengelman
Copy link
Contributor Author

@raphink - yeah, this is still using your patched catalog client.

@blackjid
Copy link

blackjid commented Oct 3, 2016

nice thanks @johnrengelman , I had the same question you ask @stack72 for the tests...

what do you think about version compatibility?? this provider should be compatible with rancher api v1 and v2 that is coming later this month?? (also I don't really know if rancher 1.2.0 will support both api versions)

@johnrengelman
Copy link
Contributor Author

I'm not sure on compatibility...there were definitely some significant changes (i.e. Environment -> Stack, etc.), Not sure if the code can logically decide between the two based on a property, might need to have different implementations.

@raphink
Copy link
Contributor

raphink commented Oct 3, 2016

rancher/go-rancher#124 seems to suggest v2 should be used as soon as it is available.

@johnrengelman
Copy link
Contributor Author

There are 2 different clients though: github.com/rancher/go-rancher/client (the original), and github.com/rancher/go-rancher/v2 (the v2 client).
Ultimately the TF provider needs to handle operating with both the 1.1.x line of Rancher and the 1.2.x when it's released.
Perhaps @ibuildthecloud could provide some direction.

@stack72
Copy link
Contributor

stack72 commented Oct 3, 2016

@johnrengelman

#9173 (comment)

Is Rancher PaaS or do we have to build infra to run these tests?

@johnrengelman
Copy link
Contributor Author

johnrengelman commented Oct 3, 2016

@stack72 there is not a hosted solution that I know of, but it's as simple as doing docker run -d rancher/server:latest to get a running version.

@stack72
Copy link
Contributor

stack72 commented Oct 3, 2016

If that's the case, then I can test the provider and add it to our nightly tests easily enough

@johnrengelman
Copy link
Contributor Author

@stack72 - great!

@raphink - I'm running the acceptance tests now and I'm having issues with them. It appears the code is configured to require access control to be enabled on Rancher, but I'm not sure that should be a requirement. I could conceive someone running an authenticated cluster on a private network (or in CI tests). Thoughts?

@blackjid
Copy link

blackjid commented Oct 3, 2016

@johnrengelman I don't think you need the access control enabled in rancher.. wWe are in fact running ci on travis against a newly created rancher server without any access control.

},
"name": &schema.Schema{
Type: schema.TypeString,
Required: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use Optinal: true here so that we can terraformize what exists without having to update to token name in the database using SQL queries because this attribute is not updatable using the API.

Copy link
Contributor Author

@johnrengelman johnrengelman Oct 19, 2016

Choose a reason for hiding this comment

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

This valuesappears to be updateable...there's an acceptance test that verifies that it is. Though, these should be optional where they are optional in the Rancher API. I'll do a pass through this tomorrow.

@stack72
Copy link
Contributor

stack72 commented Oct 6, 2016

Hi @johnrengelman

Please ping me when you feel this is worthy of final review / testing :)

P.

@johnrengelman
Copy link
Contributor Author

@stack72 will do. sorry been out sick the last few days. There's still some issues in here that I am addressing.

@stack72
Copy link
Contributor

stack72 commented Oct 6, 2016

You take as long as you need sir! :)

@blackjid
Copy link

blackjid commented Oct 7, 2016

@johnrengelman I see you added in the docs how to import these resources. The fact is the code base code for import is there, but it's only tested for the rancher_environment resource. It would be good to add the test for the other resources if we are going to maintain the import feature, which is awesome by the way.

@johnrengelman
Copy link
Contributor Author

Yup. I'm still working on this PR. When it's ready for review, I'll remove
the WIP and comment in here.
On Thu, Oct 6, 2016 at 7:44 PM Juan Ignacio Donoso notifications@github.com
wrote:

@johnrengelman https://github.com/johnrengelman I see you added in the
docs how to import these resources. The fact is the code base code for
import is there, but it's only tested for the rancher_environment
resource. It would be good to add the test for the other resources if we
are going to maintain the import feature, which is awesome by the way.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9173 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA90JEcrNJMFy8_GARWkOpc_MOrshkI-ks5qxZXkgaJpZM4KMNxa
.

@johnrengelman johnrengelman force-pushed the rancher-provider-merged branch from 993c7c9 to 35bf3da Compare October 11, 2016 21:13
@johnrengelman
Copy link
Contributor Author

@raphink @blackjid Ok, I've got everything in working except 1 thing: Creating a stack from a catalog entry.
I get the following error:

--- FAIL: TestAccRancherStack (6.60s)
    testing.go:265: Step 3 error: Error applying: 1 error(s) occurred:

        * rancher_stack.catalog: Failed to get catalog template: Unknown schema type [template]

Looks like, rancher doesn't respond to /v1-catalog/template. It wants /v1-catalog/templates.

@johnrengelman
Copy link
Contributor Author

Nevermind, when I vendor'd your fork of go-rancher I didn't get the correct branch.

@johnrengelman johnrengelman force-pushed the rancher-provider-merged branch 2 times, most recently from b467ab1 to 12958f3 Compare October 11, 2016 22:49
@raphink
Copy link
Contributor

raphink commented Oct 12, 2016

Great, thank you!

Copy link

@blackjid blackjid left a comment

Choose a reason for hiding this comment

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

Thanks for all the work integrating this.. We might have a rancher provider in the next terraform release.

Description: descriptions["access_key"],
},
"secret_key": &schema.Schema{
Type: schema.TypeString,
Required: true,
DefaultFunc: schema.EnvDefaultFunc("RANCHER_SECRET_KEY", nil),
Optional: true,

Choose a reason for hiding this comment

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

Awesome, makes total sense if we can use them from the environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point here is that Rancher doesn't require API keys. You could have a rancher server that does not have access control enabled, so it wouldn't make sense to force the user to provide these values then.

@blackjid
Copy link

What would be the next steps to get this merged?? integrating with CI on the terraform side?

@johnrengelman
Copy link
Contributor Author

I'm finishing up the stack upgrade piece today and then we should be set.

@johnrengelman johnrengelman changed the title WIP: [Provider] Rancher [Provider] Rancher Oct 14, 2016
@johnrengelman
Copy link
Contributor Author

@blackjid @raphink @stack72 this is ready for review.

For acceptance tests this is what I've done locally:

  1. docker run -d -p 8080:8080 --name rancher rancher/server:stable (runs the rancher server)
  2. curl -X POST http://localhost:8080/v1/projects/1a5/registrationTokens -d '{"name": "test"}' (create a registration token in the "Default" environment)
  3. Then run this script to add a local host to Rancher (needed for the stack acc tests)
TOKEN=`curl --silent localhost:8080/v1/projects/1a5/registrationTokens | jq -r '.data[] | select(.accountId=="1a5")'`
IMAGE=`echo $TOKEN | jq -r '.image'`
URL=`echo $TOKEN | jq -r '.registrationUrl'`
IP=`ifconfig en0 | grep "inet " | awk '{print $2}'`
MOD_URL=`echo $URL | sed 's/localhost/'${IP}'/'`

docker run -d --privileged -e CATTLE_AGENT_IP="${IP}" -v /var/run/docker.sock:/var/run/docker.sock $IMAGE $MOD_URL

d.HasChange("rancher_compose") ||
d.HasChange("environment") ||
d.HasChange("catalog_id") ||
d.HasChange("scope") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does a change in scope really require an upgrade?

Copy link
Contributor Author

@johnrengelman johnrengelman Oct 19, 2016

Choose a reason for hiding this comment

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

It doesn't. I already changed this in a later commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah great, sorry.

Delay: 1 * time.Second,
MinTimeout: 3 * time.Second,
}
s, waitErr := stateConf.WaitForState()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should Terraform finish the upgrade when there was no error, and rollback it otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know about auto-rolling back. Is there any precedent for doing that in other providers?
As for finishing the upgrade, it's an config option you can set on the resource: fd856e5#diff-52a80e386a0d139d6f657ff76d1a692cR70

@stack72
Copy link
Contributor

stack72 commented Oct 25, 2016

Hi @johnrengelman

Please can you rebase this from master? :)

P.

@johnrengelman
Copy link
Contributor Author

@raphink @blackjid - I'd love to keep your're guys' commits in here but it's problematic when rebasing and such, it makes for a ton of work....so, I'm going to squash them down into single commit. I'll mention you both int he commit message though.

Starting implementation taken from
https://github.com/platanus/terraform-provider-rancher

Commits from jidonoso@gmail.com and raphael.pinson@camptocamp.com
@blackjid
Copy link

hey @johnrengelman now problem, thanks for that.. 👍

@johnrengelman johnrengelman force-pushed the rancher-provider-merged branch from fd856e5 to 14d3eac Compare October 25, 2016 14:14
@johnrengelman
Copy link
Contributor Author

@stack72 branch is updated and i squashed down the commits.

@stack72
Copy link
Contributor

stack72 commented Oct 25, 2016

@raphink @blackjid are you both happy here with the provider and that it is all ok?

@blackjid
Copy link

LGTM

@raphink
Copy link
Contributor

raphink commented Oct 26, 2016

@stack72 fine with me.

@raphink
Copy link
Contributor

raphink commented Oct 31, 2016

Any news on that?

@johnrengelman
Copy link
Contributor Author

@stack72 anything I can do to help shepherd this along?

@raphink
Copy link
Contributor

raphink commented Nov 21, 2016

@stack72 any plan to merge this?

@raphink
Copy link
Contributor

raphink commented Nov 22, 2016

@johnrengelman @blackjid shall we get back to the platanus project for new features/bugfix until this gets merged?

@blackjid
Copy link

that make sense, but I have the feeling the merging the further changes back to this PR is not going to be easy. at least is going to be super manual.. right @johnrengelman???

@stack72
Copy link
Contributor

stack72 commented Nov 23, 2016

Hi all

Sorry I haven't been back to you all on this one - I am trying to work out how to get tests in place for this. We currently have no infrastructure running Rancher so I am trying to work out how to script it that it comes up as part of our test harnsess

I promise I will get back to this asap

P.

@blackjid
Copy link

@stack72 just as a reminder, I have some tests running in the plugin version of this provider... if that helps..
https://github.com/platanus/terraform-provider-rancher/blob/master/bin/cibuild

@mcanevet
Copy link
Contributor

@stack72 FYI we are using this branch in production for a few weeks now (since the go for merging actually) and it works pretty well.

@stack72
Copy link
Contributor

stack72 commented Dec 5, 2016

hi @johnrengelman / @raphink

This LGTM! I am going to come back and setup the testing for this to our CI server later in the week

Thanks for all the work here

Paul

@stack72 stack72 merged commit 243ecf3 into hashicorp:master Dec 5, 2016
gusmat pushed a commit to gusmat/terraform that referenced this pull request Dec 6, 2016
* Vendor Rancher Go library.

* Implement Rancher Provider.

Starting implementation taken from
https://github.com/platanus/terraform-provider-rancher

Commits from jidonoso@gmail.com and raphael.pinson@camptocamp.com
@ghost
Copy link

ghost commented Apr 19, 2020

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 Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants