-
Notifications
You must be signed in to change notification settings - Fork 178
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
chore: Upgrades federated_settings_org_config to latest SDK. #2244
Conversation
|
internal/service/federatedsettingsorgconfig/data_source_federated_settings.go
Show resolved
Hide resolved
internal/service/federatedsettingsorgconfig/data_source_federated_settings_connected_orgs.go
Show resolved
Hide resolved
@@ -60,7 +60,7 @@ func Resource() *schema.Resource { | |||
|
|||
func resourceMongoDBAtlasFederatedSettingsOrganizationConfigRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { | |||
// Get client connection. | |||
conn := meta.(*config.MongoDBClient).Atlas | |||
conn := meta.(*config.MongoDBClient).AtlasV2 | |||
|
|||
if d.Id() == "" { | |||
d.SetId("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this SetId redundant after entering the if statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
short answer: Not sure why, but it's beyond the scope of the PR which would be better to keep them focused to single topic.
long answer: might be trivial to remove but I am not sure if there's any weird difference behind it. Looking at the code:
// SetId sets the ID of the resource. If the value is blank, then the
// resource is destroyed.
func (d *ResourceData) SetId(v string) {
d.once.Do(d.init)
d.newState.ID = v
// once we transition away from the legacy state types, "id" will no longer
// be a special field, and will become a normal attribute.
// set the attribute normally
d.setWriter.unsafeWriteField("id", v)
// Make sure the newState is also set, otherwise the old value
// may get precedence.
if d.newState.Attributes == nil {
d.newState.Attributes = map[string]string{}
}
d.newState.Attributes["id"] = v
}
// Id returns the ID of the resource.
func (d *ResourceData) Id() string {
var result string
if d.state != nil {
result = d.state.ID
if result == "" {
result = d.state.Attributes["id"]
}
}
if d.newState != nil {
result = d.newState.ID
if result == "" {
result = d.newState.Attributes["id"]
}
}
return result
}
Without solid testing, it's an unnecessary risk the one of removing this statement (and, as I said, not related to the title of this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't be afraid of changing any code as this perpetuates code that nobody knows why it is there, but agree that that we need to have good tests as a safety net.
agreed it doesn't belong strictly to this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two similar cases in the code base where there is an if d.Id() == "" {d.setId("")}
- we should figure why this is done and make sure there are tests around it before changing it. @lantoli do you want to create a Jira issue for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually let me just create that Jira issue, thank you for calling that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
internal/service/federatedsettingsorgconfig/resource_federated_settings_connected_org.go
Show resolved
Hide resolved
internal/service/federatedsettingsorgconfig/resource_federated_settings_connected_org.go
Outdated
Show resolved
Hide resolved
federation_settings_id = %[1]q | ||
org_id = %[2]q | ||
domain_restriction_enabled = false | ||
domain_allow_list = ["mydomain.com"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seeing this test data change it looks like we're not checking that param in the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch @lantoli! Took two actions:
- restored to original values (I was making some tests initially because it wasn't working).
- Added more test checks to cover the other attributes.
Let me know if this works for you.
…_settings_connected_org.go Co-authored-by: Leo Antoli <430982+lantoli@users.noreply.github.com>
internal/service/federatedsettingsorgconfig/data_source_federated_settings_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Please include a summary of the fix/feature/change, including any relevant motivation and context.
Link to any related issue(s):
Type of change:
Required Checklist:
Further comments