Skip to content
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

Add roles resource and permissions datasource #753

Merged
merged 22 commits into from
Nov 23, 2020
Merged

Add roles resource and permissions datasource #753

merged 22 commits into from
Nov 23, 2020

Conversation

zippolyte
Copy link
Contributor

No description provided.

@zippolyte zippolyte requested a review from a team as a code owner November 17, 2020 13:20
pietdaniel
pietdaniel previously approved these changes Nov 19, 2020
Copy link

@pietdaniel pietdaniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, mostly questions, my terraform provider knowledge is still rather fresh so take the approval with a grain of salt

url: https://api.datadoghq.com/api/v2/permissions
method: GET
response:
body: '{"data":[{"type":"permissions","id":"984a2bd4-d3b4-11e8-a1ff-a7f660d43029","attributes":{"name":"admin","display_name":"Privileged

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when we add new permissions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't test for the exact set of returned permissions, so this well still pass as new permissions are added

State: resourceDatadogRoleImport,
},
CustomizeDiff: customdiff.ValidateValue("permission", validatePermissionsUnrestricted),
Schema: map[string]*schema.Schema{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this include the role UUID?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ID's are specific in terraform. They are not in the schema, but they are set in the state (look for SetId()) and user can reference it in other resources with resource_name.id

@zippolyte zippolyte requested a review from a team as a code owner November 20, 2020 12:10
@@ -100,6 +103,12 @@ type ProviderConfiguration struct {
DatadogClientV2 *datadogV2.APIClient
AuthV1 context.Context
AuthV2 context.Context

now func() time.Time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not directly?

Suggested change
now func() time.Time
Now func() time.Time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't even think of that haha. Guess it felt more "go" the other way, but yeah, I guess this works too

Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Left a few inline notes/comments.


Schema: map[string]*schema.Schema{
// Computed values
"permissions": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we'd want to add additional fields to this later? I'm wondering if it'd be useful to use schema like:

permissions: [
    {
        "name": <name>
         "id": <id>
    }
]

but that may be a bit of overkill here, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I don't know. And I do need this mapping so that it's practical for user to reference in other resources. A list wouldn't allow that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're right, we don't want users to have to care about the order of the list...

I just want to make sure will be able to add new fields in the future without compatibility issues if needed. (But maybe we won't need it) Another possibility may be this, though I'm unsure if that means you'd need to know the hash

    {
        "name": {
            "id": <id>,
            "future_fields": ....,
         }
    }
]```

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in a map I can only have primitive types though
Elem represents the element type. For a TypeMap, it must be a *Schema with a Type that is one of the primitives: TypeString, TypeBool, TypeInt, or TypeFloat

datadog/data_source_datadog_permissions.go Outdated Show resolved Hide resolved
docs/resources/role.md Show resolved Hide resolved
docs/resources/role.md Outdated Show resolved Hide resolved
datadog/resource_datadog_role_test.go Show resolved Hide resolved
datadog/resource_datadog_role_test.go Outdated Show resolved Hide resolved
@@ -509,7 +508,7 @@ func resourceDatadogMonitorRead(d *schema.ResourceData, meta interface{}) error

// Ignore any timestamps in the past that aren't -1 or 0
for k, v := range configSilenced {
if v.(int) < int(time.Now().Unix()) && v.(int) != 0 && v.(int) != -1 {
if v.(int) < int(providerConf.Now().Unix()) && v.(int) != 0 && v.(int) != -1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I fully understand what this change addresses?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

problem in replaying tests, since we do a match on the body now. So we need to be able to set the frozen clock in the test, this allows us to do that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh ok, so no functional user difference, just for tests 👍

Required: true,
Description: "ID of the permission to assign.",
},
"name": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this mostly here for the ability to reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I'm not sure about this one though. At first I had a simple string list of IDs, then I had to add it for my first try at implanting diffSupressFunc that i didn't end up doing, but kept this as is 🤷

Comment on lines +48 to +49
// Get a list of all permissions, to ignore restricted perms
if validPermissions == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 💯

@nmuesch nmuesch self-requested a review November 20, 2020 22:47
Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant to "request changes" to discuss my above notes 🤦

Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, This looks good to me 👍

@zippolyte zippolyte merged commit ff705c1 into master Nov 23, 2020
@zippolyte zippolyte deleted the hippo/rr branch November 23, 2020 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants