keycloak_openid_client remove merge with existing config on import

[chrismilson]

Fixes #1007

[chrismilson]

Not obvious by the error messages why the acceptance tests failed - some help would be greatly appreciated :)

[zam6ak]

any chance this can get into next release?

[chrismilson]

Oh dear...

[sschu]

@chrismilson Have you tried running the tests locally?

[chrismilson]

@sschu I have not been able to do that - I have had trouble setting up the development environment locally

I should be able to run tests locally over the weekend though.

[chrismilson]

I was able to run tests locally. Had a little difficulty setting up the environment, but I'll put that info in a separate issue.

Should be ready for review, now @sschu. Thank you for your patience!

[chrismilson]

@sschu anything else I should do before merge?

[sschu]

@chrismilson Sorry for the delay. I would still like to reproduce this first and also look at how this is done for other resources. I need a bit more time since I have some pressing issues on my plate currently.

[chrismilson]

No worries :) take your time

I'm also happy to allocate some time to this keycloak terraform project - let me know if there's anything I can do

[sschu]

@chrismilson So I finally had time to look at this issue, sorry it took so long. I can reproduce it and I think you analysis of whats going on is correct.

What is still puzzling me is why would you use the existing configuration of the client at all when updating it? What you really want after import is to have the configuration you declared and default values for anything you haven't declared. But I am afraid I might be missing something here.

The thing is, the merge also exists for keycloak_role and I would like to keep this consistent. Do you have any idea why the merge could be here?

@mrparkers Do you happen to remember why you did it that way?

[mrparkers]

@sschu If I recall correctly, the original problem we were trying to solve with the import argument is to deal with situations where creating some resource in Keycloak resulted in other resources that are created as a side effect, and you want to manage those resources with Terraform. So if you create a realm, it creates a bunch of clients, and if you want to manage those clients in Terraform you're stuck doing a bunch of terraform import commands to get everything into state, and this workflow is not very friendly for automation. With the import argument, you can essentially do terraform import automatically.

At the time, I made the decision that this functionality should be very similar to how terraform import works, and it has the same behavior. If you create some resource manually (like in the Keycloak UI), and you write some Terraform code that does not exactly match the resource you created, terraform import will only import this resource into state, and not change anything about the resource. So a subsequent terraform plan would show differences. I wanted the import argument to work the same way.

Whether or not it should work this way is probably still a subject for debate. The obvious downside to this decision is an initial terraform apply followed by a terraform plan shows changes, which is not something you would normally expect.

[sschu]

@mrparkers Thanks for elaborating, seems like you have a good memory! ;) I understand the intention now which is to prevent doing any changes to the existing configuration without being alerted of this. Need to think about this a little more...

[chrismilson]

Sounds like there are two apparently conflicting requirements:

• Running terraform apply should be idempotent; whatever state we would expect resources to be in after the second apply, they should be in after the first apply (assuming no config change).
• When importing configuration, any changes occurring to the original resource should be made explicit.

The original implementation fails on the first point, but this PR's implementation currently fails on the second point. Perhaps a good starting place would be some unit tests to model both of these situations. I'll take that as an action.

I have a couple of ideas for how we might be able to implement something that could satisfy both conditions, but would need to do a bit more exploration first.

[chrismilson]

Is there a better name to use than "import" if the expectation is that it is only intended to be used on resources created implicitly? (but that should also be tracked by terraform)

The existing terraform import and import blocks serve valid purposes, and should, I imagine, probably be used for importing resources that genuinely need to be "imported" - in the terraform-resource-lifecycle sense of the word. If the only reason we would use the import = true field is because it is for resources created implicitly and thus non-existent at plan time, perhaps we should call the field created_implicitly or keycloak_default or something?

If we treat it that way, then the onus to satisfy the second requirement - explicitly show changes from pre-existing values, because that is how a terraform import would behave - feels less.

[sschu]

@chrismilson I was thinking in similar directions, like would it be possible to already show a diff on the original apply when Terraform says a resource would be created. But I could imagine that's not possible since resource creating normally looks different. I also agree on the naming of the parameter, but I would probably not change it this time since it's a breaking change.

[chrismilson]

I'm pretty sure we wont be able to come up with something that always shows the difference on the first plan, just because in some situations, the resource to be imported will only exist at apply time. (e.g. account-console client if the plan includes the creation of the realm as well)

If you limit your scope to situations when the resource to import does exist at the first plan, the import = true field doesn't get an opportunity from terraform to read/refresh the state of the existing object, so you get stuck there. Im assuming you can't mutate values in the state in the first call to the resource's Plan method, even though in this case you would be able to make the GetClientByClientId call.

What if we deprecate import, create a newly named boolean field, and until the next major release both fields have the same effect, you just get a warning if you use "import" to switch to the new naming. Let me know if I'm being way too gung-ho on the whole naming thing too. In theory we just need to update the documentation for the field if we decide what the provider's opinions are about its intended use.