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.

Sign up for GitHub

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

Jump to bottom

Add fields to azurerm_*_cost_management_export: partition_data and export_data_options.data_version #26219

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add fields to azurerm_*_cost_management_export: partition_data and export_data_options.data_version #26219

wants to merge 2 commits into from
+102 −31

Conversation

joshk0
Copy link

@joshk0 joshk0 commented Jun 4, 2024

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

Description

Add fields to azurerm_*_cost_management_export: partition_data and export_data_options.version.

export_data_options.version maps to export.definition.dataset.configuration.dataVersion - No default. Versions can proliferate and even though we can collect a large list of possible versions it's better to leave it free form.

partition_data maps to export.partition_data. Defaults to false.

PR Checklist

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
    For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

  • I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
  • I have written new tests for my resource or datasource changes & updated any relevent documentation.
  • I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
  • (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Fixes #23747

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

@joshk0 joshk0 changed the title Exports add fieldsdd fields to azurerm_*_cost_management_export: partition_data and export_data_options.data_version Add fields to azurerm_*_cost_management_export: partition_data and export_data_options.data_version Jun 4, 2024
@joshk0
Copy link
Author

joshk0 commented Jun 4, 2024

Please let me know if I need to run the live Azure acceptance tests myself in order to proceed.

@github-actions github-actions bot added size/M and removed size/XL labels Jun 4, 2024
@joshk0
Copy link
Author

joshk0 commented Jun 12, 2024

Hello, is it possible to get a review for this?

katbyte
katbyte requested changes Jun 21, 2024
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

thanks for the pr @joshk0 - for booleans we add _enabled to the end of them to mark them as bools. once that change is made i think this is good to go

website/docs/r/billing_account_cost_management_export.html.markdown Outdated Show resolved Hide resolved
@joshk0
Copy link
Author

joshk0 commented Jun 22, 2024

@katbyte, done. Please have a look. Thank you!

@joshk0
Add fields to azurerm_*_cost_management_export: partition_data an…
41769c3 
…d `export_data_options.version`

`export_data_options.version` maps to
`export.definition.dataset.configuration.dataVersion` No default. Versions can
proliferate and even though we can collect a large list of possible versions
it's better to leave it free form.

`partition_data` maps to `export.partition_data`. Defaults to false.
@joshk0
Rename partition_data to partition_data_enabled
e60895b
@joshk0
Copy link
Author

joshk0 commented Jul 3, 2024 •
edited
Loading

I have rebased the branch to include the new go-azure-sdk. maybe it will work now?

Could someone please approve the workflows?

stephybun
stephybun requested changes Jul 8, 2024
View reviewed changes
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @joshk0. There are some minor things that need fixing up to bring this more in-line with other resources in the provider.

Most resources have a basic and a complete test configuration, where the basic only includes properties that are Required and the complete configuration includes properties that are Optional. I see that we're missing a complete test config here, do you think you could look into adding that so we can validate the partition_data_enabled property?

internal/services/costmanagement/export_resource_base.go
"partition_data_enabled": {
Type: pluginsdk.TypeBool,
Optional: true,
Required: false,
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to specify false for Required since it's implied by Optional being set to true

Suggested change
Required: false,

internal/services/costmanagement/export_resource_base.go
@@ -100,10 +108,17 @@ func (br costManagementExportBaseResource) arguments(fields map[string]*pluginsd
string(exports.TimeframeTypeTheLastMonth),
string(exports.TimeframeTypeWeekToDate),
string(exports.TimeframeTypeMonthToDate),
string(exports.TimeframeTypeMonthToDate),
Copy link
Member

Choose a reason for hiding this comment

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

We already support this value above so this would be a duplicate

Suggested change
string(exports.TimeframeTypeMonthToDate),

internal/services/costmanagement/export_resource_base.go
"data_version": {
Type: pluginsdk.TypeString,
Optional: true,
Required: false,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Required: false,

internal/services/costmanagement/export_resource_base.go
// TODO Use value from SDK after https://github.com/Azure/azure-rest-api-specs/issues/23707 is fixed
"TheLast7Days",
}, false),
},

"data_version": {
Copy link
Member

Choose a reason for hiding this comment

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

Is there some validation we could add for this property? Even just a validation.StringIsNotEmpty?

internal/services/costmanagement/export_resource_base.go
Comment on lines +192 to +194
if partitionData := props.PartitionData; partitionData != nil {
metadata.ResourceData.Set("partition_data_enabled", *partitionData)
}
Copy link
Member

Choose a reason for hiding this comment

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

We could use pointer.From here to do the nil-check

Suggested change
if partitionData := props.PartitionData; partitionData != nil {
metadata.ResourceData.Set("partition_data_enabled", *partitionData)
}
metadata.ResourceData.Set("partition_data_enabled", pointer.From(props.PartitionData))

website/docs/r/resource_group_cost_management_export.html.markdown
@@ -59,6 +61,8 @@ The following arguments are supported:

* `resource_group_id` - (Required) The id of the resource group on which to create an export. Changing this forces a new resource to be created.

* `partition_data_enabled` - (Optional) Whether data will be partitioned into smaller files to facilitate consumption.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `partition_data_enabled` - (Optional) Whether data will be partitioned into smaller files to facilitate consumption.
* `partition_data_enabled` - (Optional) Whether data will be partitioned into smaller files to facilitate consumption. Changing this forces a new resource to be created. Defaults to `false`.

website/docs/r/subscription_cost_management_export.html.markdown
@@ -61,6 +63,8 @@ The following arguments are supported:

* `subscription_id` - (Required) The id of the subscription on which to create an export. Changing this forces a new resource to be created.

* `partition_data_enabled` - (Optional) Whether data will be partitioned into smaller files to facilitate consumption.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `partition_data_enabled` - (Optional) Whether data will be partitioned into smaller files to facilitate consumption.
* `partition_data_enabled` - (Optional) Whether data will be partitioned into smaller files to facilitate consumption. Changing this forces a new resource to be created. Defaults to `false`.

internal/services/costmanagement/export_billing_account_resource_test.go
@@ -143,15 +143,17 @@ resource "azurerm_billing_account_cost_management_export" "test" {
recurrence_type = "Monthly"
recurrence_period_start_date = "%sT00:00:00Z"
recurrence_period_end_date = "%sT00:00:00Z"
partition_data_enabled = true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
partition_data_enabled = true

internal/services/costmanagement/export_billing_account_resource_test.go
export_data_storage_location {
container_id = azurerm_storage_container.test.resource_manager_id
root_folder_path = "/root"
}
export_data_options {
type = "Usage"
time_frame = "TheLastMonth"
data_version = "2019-11-01"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data_version = "2019-11-01"

internal/services/costmanagement/export_resource_group_resource_test.go
@@ -171,14 +173,16 @@ resource "azurerm_resource_group_cost_management_export" "test" {
recurrence_type = "Monthly"
recurrence_period_start_date = "%sT00:00:00Z"
recurrence_period_end_date = "%sT00:00:00Z"
partition_data_enabled = true
Copy link
Member

Choose a reason for hiding this comment

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

This would trigger the recreation of the resource so this needs to be removed from the update test config.

Suggested change
partition_data_enabled = true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katbyte katbyte katbyte requested changes

@stephybun stephybun stephybun requested changes

Assignees
No one assigned
Labels
dependencies documentation service/cost-management service/systemcentervirtualmachinemanager size/M waiting-response
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for partition_data in azurerm_subscription_cost_management_export
3 participants
@joshk0 @katbyte @stephybun