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 support for business units. #498

Closed

Conversation

VictorAvelar
Copy link
Contributor

@VictorAvelar VictorAvelar commented May 24, 2024

This PR adds support for business units.

It is a follow up to #431 and addresses the previously recommended changes. It also partially addresses #443.

NEW FEATURES

  • Introduces the Company resource
  • Introduces the Division resource

@VictorAvelar VictorAvelar marked this pull request as ready for review May 24, 2024 12:43
@VictorAvelar VictorAvelar requested a review from a team as a code owner May 24, 2024 12:43
@demeyerthom
Copy link
Member

@VictorAvelar great! I will take a look this week and test it out!

Copy link
Member

@demeyerthom demeyerthom left a comment

Choose a reason for hiding this comment

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

I primarily looked at the schema for the resources, and left some comments there.

I also tried running the example in resource.tf, but it broke even after fixing the issues I also left in the comments.

Some other things I hope you can add:

  • please run task docs. It will generate documentation based on the schemas
  • please split the examples/resources/commercetools_business_unit/resource.tf to examples/resources/commercetools_business_unit_company/resource.tf and examples/resources/commercetools_business_unit_division/resource.tf. This will add these examples to the docs also. It is also nice to add resources for related stuff like commercetools_store in the same example, so users can more easily see what can be attached

// Only available for division business units as the Company
// business unit has no parent unit and must always be the Top Level Unit.
parent_unit {
key = commercetools_business_unit_company.acme-company.key
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
key = commercetools_business_unit_company.acme-company.key
key = commercetools_business_unit_company.acme_company.key

}

// Schema implements resource.Resource.
func (b *divisionResource) Schema(_ context.Context, req resource.SchemaRequest, res *resource.SchemaResponse) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this one (and the related model and tests) to a different folder? So resources/business_unit_company and resources/business_unit_division? We try to keep it one folder per resource

name = "Willie Coyote - Traps for Roadrunners"
status = "Active"
contact_email = "acme-traps@example.com"
store_mode = "FromParent"
Copy link
Member

Choose a reason for hiding this comment

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

I think this one is not actually part of the definition?

// business unit has no parent unit and must always be the Top Level Unit.
parent_unit {
key = commercetools_business_unit_company.acme-company.key
type_id = "company"
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
type_id = "company"
type_id = "Company"

default_billing_address_id = "acme-business-unit-address"
}

resource "commercetools_business_unit_division" "acme-willie-coyote" {
Copy link
Member

Choose a reason for hiding this comment

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

The address are also required, might be nice to add it to the example

Description: "User-defined unique identifier of the Business Unit",
Required: true,
},
"type_id": schema.StringAttribute{
Copy link
Member

Choose a reason for hiding this comment

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

I think this one is always business-unit, so this can be removed (and the validation is incorrect). Instead we should add id as an optional input, and make key optional also

},
},
},
"stores": schema.ListNestedBlock{
Copy link
Member

Choose a reason for hiding this comment

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

See comments above

},
},
},
"inherited_associates": schema.ListNestedBlock{
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this one will have much value in the context of a terraform provider (it is eventually consistent and a result of other resources made). Do you have a usecase where this might be useful?

stringvalidator.OneOf(BusinessUnitActive, BusinessUnitInactive),
},
},
"associate_mode": schema.StringAttribute{
Copy link
Member

Choose a reason for hiding this comment

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

This one is missing the validation, and can be defaulted to ExplicitAndFromParent (and thus not required)

res.Schema = schema.Schema{
Description: "Business Unit type to represent the top level of a business. Contains specific fields and values that differentiate a Company from the generic BusinessUnit.\n\n" +
"See also the [Business Unit API Documentation](https://docs.commercetools.com/api/projects/business-units",
Attributes: map[string]schema.Attribute{
Copy link
Member

Choose a reason for hiding this comment

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

storeMode is missing as an option (and can be optional and default to FromParent

@demeyerthom
Copy link
Member

Hi @VictorAvelar! If that is ok with you I will grab your branch and finish up the code! We have an internal use case for these features, so I can use your PR as a base for that :)

@VictorAvelar
Copy link
Contributor Author

Please go ahead, I couldn't find the time yet to finish it.

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.

2 participants