Swap out internal client for new external client - SAML Groups #123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
micahkemp-splunk
requested review from
anushjay,
guohuang,
clarafeb1,
dtwersky and
rfrey-splunk
micahkemp-splunk
force-pushed
the
external-saml
branch
6 times, most recently
from
e1b2075 to
3a20107
Compare
Comment on lines
-93
to
+124
| err := (*provider.Client).UpdateAdminSAMLGroups(d.Id(), adminSAMLGroupsObj) | ||
| err := (*provider.Client).UpdateAdminSAMLGroups(d.Get("name").(string), adminSAMLGroupsObj) |
There was a problem hiding this comment.
Because the legacy functions expected the Id to be the name, avoid potential issues caused by inconsistency between the two providers by always using the name when updating/deleting SAML groups.
Comment on lines
-103
to
+134
| resp, err := (*provider.Client).DeleteAdminSAMLGroups(d.Id()) | ||
| resp, err := (*provider.Client).DeleteAdminSAMLGroups(d.Get("name").(string)) |
There was a problem hiding this comment.
Because the legacy functions expected the Id to be the name, avoid potential issues caused by inconsistency between the two providers by always using the name when updating/deleting SAML groups.
anushjay
reviewed
May 26, 2022
micahkemp-splunk
changed the title
micahkemp-splunk
changed the title
micahkemp-splunk
force-pushed
the
external-saml
branch
9 times, most recently
from
54f7a69 to
858cabb
Compare
micahkemp-splunk
force-pushed
the
external-saml
branch
2 times, most recently
from
7dab85d to
8ea0202
Compare
micahkemp-splunk
commented
Jun 8, 2022
splunk/entry_crud.go
Outdated
Comment on lines
31
to
36
| // GetOk only returns true if the fetched value is not the zero value for its type, | ||
| // so we can only determine if use_legacy_client was explicitly true. but because | ||
| // true is our default value, we know that it can only be false if explicitly set. | ||
| if ok || !resourceLegacyClient { | ||
| return resourceLegacyClient | ||
| } |
There was a problem hiding this comment.
I'm working to validate this comment and logic is correct, after behavior I saw while testing similar code yesterday.
micahkemp-splunk
force-pushed
the
external-saml
branch
from
ab80dd7 to
c54de7b
Compare
micahkemp-splunk
force-pushed
the
external-saml
branch
from
c54de7b to
b55ed6f
Compare
micahkemp-splunk
force-pushed
the
external-saml
branch
from
b55ed6f to
ee7b2b4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is the start of the migration from the internally-included Splunk client for the newly available external client. The external client is added alongside the legacy client, and can be used by setting either the appropriate provider (
use_client_default) or resource (use_client) configuration toexternal.The external client handles CRUD operations with standard Create/Read/Update/Delete functions, which deduplicates a lot of code, and provides a single location to implement fixes when issues are discovered. Migrating this Terraform provider to use it should solve a lot of the error handling and drift issues currently present.
The interesting work is found in 5a34349, which adds composable CRUD function creation, simplifying logic and reducing repetition.
The remaining commits are in support of this swap.
As this PR adds the ability to use the external client, but by default does not change existing behavior, it can be merged and tagged without a major version bump. After all existing resources permit use of the external client, a new PR can be submitted to make the default behavior to use the external client, which would justify a major version bump.