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

Change Aggregation Property resource to Aggregation Properties resource #99

Conversation

Tankilevitch
Copy link
Contributor

@Tankilevitch Tankilevitch commented Jan 14, 2024

Description

What -

  • Replaced aggregation property resource to aggregation_properties resources

Why -

  • To avoid race condition when trying to create multiple aggregation properties for one blueprint, causing some of them to get overridden due to the terraform apply running in parallel

How -

  • By changing the resource to hold a list of aggregation properties it allows to handle all in action

Type of change

Please leave one option from the following and delete the rest:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Non-breaking change (fix of existing functionality that will not change current behavior)
  • Documentation (added/updated documentation)

@Tankilevitch Tankilevitch changed the title fix already exists nil pointer error and fix import state Fix already exists nil pointer error and fix import state Jan 14, 2024
@Tankilevitch Tankilevitch changed the title Fix already exists nil pointer error and fix import state Change Aggregation Property resource to Aggregation Properties resource Jan 14, 2024
Copy link
Contributor

@talsabagport talsabagport left a comment

Choose a reason for hiding this comment

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

Great work! left some comments 🚀

return
}

//blueprintIdentifier := state.BlueprintIdentifier.ValueString()
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the comment

@@ -66,6 +66,27 @@ func (c *PortClient) UpdateBlueprint(ctx context.Context, b *Blueprint, id strin
return &pb.Blueprint, nil
}

func (c *PortClient) PatchBlueprint(ctx context.Context, b *Blueprint, id string) (*Blueprint, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not being used (?)

return
}

err = refreshAggregationPropertyState(state, bp.AggregationProperties)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why refresh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

return
}

err = refreshAggregationPropertyState(state, bp.AggregationProperties)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why refresh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

for aggregationPropertyIdentifier, aggregationProperty := range aggregationProperties {

state.Properties[aggregationPropertyIdentifier] = &AggregationPropertyModel{
types.StringPointerValue(aggregationProperty.Title),
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to specify the struct keys explicitly

}

if aggregationProperty.Query != nil {
query, err := json.Marshal(aggregationProperty.Query)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use GoObjectToTerraformString util

"func": "count",
"calculationBy": "entities",
}
} else if aggregationProperty.Method.AverageEntities != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

The elses are redundant no? Can be just serial ifs

MarkdownDescription: "Function to count the entities of the target entities",
Optional: true,
Validators: []validator.Bool{
boolvalidator.ConflictsWith(path.MatchRelative().AtParent().AtName("average_entities")),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want ExactlyOneOf and not ConflictsWith, because otherwise you also support an empty method. And also using ExactlyOneOf you can specify the validators once (maybe right also with ConflictsWith)

icon = "Terraform"
identifier = "%s"
description = ""
relations = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Something seems not right with the identation of relations throughout the file, it seems like because here the indentation is with spaces, and other places with tabs

parentBlueprintIdentifier := utils.GenID()
childBlueprintIdentifier := utils.GenID()
var testAccActionConfigCreate = fmt.Sprintf(`
resource "port_blueprint" "parent_blueprint" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can extract the parent and child blueprint definition and reuse, it will be more difficult to maintain it in the future like this

@Tankilevitch Tankilevitch merged commit e988686 into main Jan 15, 2024
3 checks passed
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