[datadog_app_builder_app_json] APPS-2248 & APPS-2144 App Builder Terraform Provider #2723
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
tyffical
force-pushed
the
tiffany.trinh/apps-2144-terraform
branch
2 times, most recently
from
c37ed5c to
8006937
Compare
tyffical
force-pushed
the
tiffany.trinh/apps-2144-terraform
branch
2 times, most recently
from
0e763b7 to
422a9c9
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
tyffical
force-pushed
the
tiffany.trinh/apps-2144-terraform
branch
from
e7878b0 to
25dde39
Compare
tyffical
changed the title
tyffical
changed the title
tyffical
force-pushed
the
tiffany.trinh/apps-2144-terraform
branch
from
ce431ec to
19f3d2b
Compare
tyffical
changed the title
tyffical
changed the title
tyffical
force-pushed
the
tiffany.trinh/apps-2144-terraform
branch
from
07cf33b to
02d8ac5
Compare
embeaken
reviewed
Feb 6, 2025
| @@ -409,6 +411,16 @@ func defaultConfigureFunc(p *FrameworkProvider, request *provider.ConfigureReque | |||
| ddClientConfig.SetUnstableOperationEnabled("v2.DeleteAWSAccount", true) | |||
| ddClientConfig.SetUnstableOperationEnabled("v2.GetAWSAccount", true) | |||
|
|
|||
| // Enable unstable operations for the App Builder API | |||
There was a problem hiding this comment.
before public release let's make sure the endpoints are on v2
| resp, httpResp, err := r.Api.CreateApp(r.Auth, *createRequest) | ||
| if err != nil { | ||
| if httpResp != nil { | ||
| // error body may have useful info for the user |
There was a problem hiding this comment.
just an observation from my testing - you should make sure your API doesn't expose any internal errors or they will show up in TF errors as well
drichards-87
previously approved these changes
Feb 6, 2025
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
tyffical
force-pushed
the
tiffany.trinh/apps-2144-terraform
branch
2 times, most recently
from
0391be0 to
6807aa8
Compare
tyffical
changed the title
tyffical
changed the title
tyffical
force-pushed
the
tiffany.trinh/apps-2144-terraform
branch
from
58c3dac to
96dded8
Compare
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| } | ||
|
|
||
| func appJSONEqual(s1, s2 string) (bool, error) { | ||
| s1, err := normalizeAppBuilderAppJSONString(s1) |
There was a problem hiding this comment.
🔵 Code Quality Violation
Avoid modifying function parameters (...read more)
Assigning new values to function parameters exhibits several bad coding practices and should be avoided for several reasons:
- Redefining parameters: The code redefines a parameter within the function body by assigning a new value. This is considered a bad practice because it can lead to confusion and make the code harder to understand. Modifying a function parameter in this manner breaks the expected behavior and can cause unexpected side effects. It is generally best to treat function parameters as read-only values and avoid reassigning them.
- Shadowing variables: The code further exacerbates the issue by using the short variable declaration
:=to define a new variable within the function body. This shadows the original parameter, making it inaccessible within the function. Shadowing variables can cause confusion and make the code harder to reason about. It is better to use distinct variable names to maintain clarity and avoid any unintended side effects.
To write more maintainable and understandable code, it is advisable to adhere to the following practices:
- Avoid redefining function parameters.
- Use descriptive and unambiguous variable names.
- Avoid shadowing variables.
- Maintain consistency in variable references.
By following these best practices, the code becomes more readable and easier to manage and avoids introducing unnecessary complexity and confusion.
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.
Motivation
app_jsoninputaction_query_ids_to_connection_idsmapReferences
Testing
Wait Before Merging
datadog-api-client-goto latest version to include above changesRELEASING.md: https://github.com/DataDog/terraform-provider-datadog/blob/master/RELEASING.mdOpen Questions
UPDATE: name will be
datadog_app_builder_appb/c we decided offline we'll just keep this one resource and add extra TF optional fields if customer feedback suggests it is necessaryRESOLVED: name will bedatadog_app_builder_app_jsonCurrently, this resource is calleddatadog_app. The plan is, in the future, updating this resource with extra structs in order to build an app in TF with TF syntax, strongly typing structs likequeriesandcomponents.Alternatively, what about creating a separate resource? This would help with making the code cleaner and disambiguate the two use cases. For instance, this resource would instead bedatadog_app_jsonsimilar to how dashboards has adatadog_dashboard_json, and the non-json usage would be calleddatadog_app, like how dashboards also hasdatadog_dashboard.