Revert back "import" property and replace it with native import block. Fixes (#870) (#1007) and unblocks (#1267)
#1359
Conversation
mikhail-putilov
changed the title
import block. Fixes (#870) (#1267)import block. Fixes (#870) (#1007) and unblocks (#1267)
mikhail-putilov
force-pushed
the
bugfix/870
branch
from
d6b35a1 to
99655e9
Compare
| - postgres:/var/lib/postgresql | ||
| openldap: | ||
| image: bitnami/openldap:2.6 | ||
| image: bitnamilegacy/openldap:2.6 |
There was a problem hiding this comment.
https://hub.docker.com/u/bitnamilegacy
bitnami is no longer free
| } | ||
| testAccProvider = KeycloakProvider(keycloakClient) | ||
|
|
||
| testAccProvider.ResourcesMap["keycloak_openid_client"].DeleteContext = func(ctx context.Context, data *schema.ResourceData, i interface{}) diag.Diagnostics { |
There was a problem hiding this comment.
I need to make this part more clearer.
Default clients cannot be deleted. Keycloak returns 400. Terraform acceptance test library simply can't handle this situation. One way what I found to overcome this is to replace DeleteContext function in acceptance tests only.
I believe, we must not alter the behavior of standard response of KeyCloak when client is deleted, nor hide that fact that KeyCloak response is 400 in the production code.
For now, I left a comment in git the commit for this.
| Optional: true, | ||
| Default: false, | ||
| }, | ||
| "import": { |
There was a problem hiding this comment.
This is breaking change.
I think depending what maintainers say, we either keep it for backward-compatibility (making it to affect nothing), or remove completely.
| return nil | ||
| } | ||
|
|
||
| func (keycloakClient *KeycloakClient) SearchOpenidClientExact(ctx context.Context, realmId string, clientId string) (*OpenidClient, error) { |
There was a problem hiding this comment.
This is needed to import clients by their respective ClientId
| "client_offline_session_idle_timeout": { | ||
| Type: schema.TypeString, | ||
| Computed: true, | ||
| Optional: true, |
There was a problem hiding this comment.
this is part of a revert commit I was telling in the description
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: testKeycloakOpenidClient_import("non-existing-client", true), | ||
| ExpectError: regexp.MustCompile("Error: openid client with name non-existing-client does not exist"), |
There was a problem hiding this comment.
previous error message was not consistent with KeyCloak glossary: it must be "Client ID", not "name"
| { | ||
| Config: testKeycloakOpenidClient_import("account", true), | ||
| Check: testAccCheckKeycloakOpenidClientExistsWithEnabledStatus("keycloak_openid_client.client", true), | ||
| Config: testKeycloakOpenidClient_import("account", false), |
There was a problem hiding this comment.
Here, I verify that disabling account client is possible (instead of previous test that keeps it in the same enabled state)
| `, testAccRealm.Realm, clientId, clientId, enabled) | ||
| } | ||
|
|
||
| func testKeycloakOpenidClient_import_postcondition(clientId string, enabled bool) string { |
| }) | ||
| } | ||
|
|
||
| func TestAccKeycloakOpenidClient_import_postcondition(t *testing.T) { |
| }) | ||
| } | ||
|
|
||
| func TestAccKeycloakOpenidClient_AccessToken_removed(t *testing.T) { |
mikhail-putilov
force-pushed
the
bugfix/870
branch
from
25f20ef to
093f7c3
Compare
Signed-off-by: Mikhail Putilov <[email protected]>
Signed-off-by: Mikhail Putilov <[email protected]>
…1267) Signed-off-by: Mikhail Putilov <[email protected]>
…is undeletable (KeyCloak returns http status 400) Terraform acceptance test library simply can't handle this situation. One way that I found to overcome this is to replace the `DeleteContext` function in acceptance tests only. I believe we must not alter the behavior of the standard response of KeyCloak when a client is deleted, nor hide that fact that KeyCloak response is 400 in the production code. Signed-off-by: Mikhail Putilov <[email protected]>
Signed-off-by: Mikhail Putilov <[email protected]>
Signed-off-by: Mikhail Putilov <[email protected]> Signed-off-by: Mikhail Putilov <[email protected]>
…ient Signed-off-by: Mikhail Putilov <[email protected]> Signed-off-by: Mikhail Putilov <[email protected]>
Signed-off-by: Mikhail Putilov <[email protected]> Signed-off-by: Mikhail Putilov <[email protected]>
…se it deals with common default resource: account openid client and default roles Signed-off-by: Mikhail Putilov <[email protected]> Signed-off-by: Mikhail Putilov <[email protected]>
…rs because it deals with common default resource: account openid client Signed-off-by: Mikhail Putilov <[email protected]> Signed-off-by: Mikhail Putilov <[email protected]>
…ID client in docs Signed-off-by: Mikhail Putilov <[email protected]> Signed-off-by: Mikhail Putilov <[email protected]>
Signed-off-by: Mikhail Putilov <[email protected]> Signed-off-by: Mikhail Putilov <[email protected]>
Signed-off-by: Mikhail Putilov <[email protected]> Signed-off-by: Mikhail Putilov <[email protected]>
mikhail-putilov
force-pushed
the
bugfix/870
branch
from
093f7c3 to
af9a739
Compare
…ward template Signed-off-by: Mikhail Putilov <[email protected]>
1 participant
I did it to fix two issues:
fixes #870
fixes #1007
And while I was working on it, it was natural to unblock this one: #1267
Reasoning:
I personally can't see a reason why
importattribute is introduced. Not only it introduces bugs, but it is custom behavior which is not consistent with other providers.import { id = ..., to = ... }block is a native way to deal with default resources.If I am wrong, I am open to hear your thoughts.
Imo, I need to deprecate
importattribute according to https://developer.hashicorp.com/terraform/plugin/framework/deprecations instead of removing it. If you agree, I will rework this PRTodo:
importattribute makes sense