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

Added a parameters for LoadBalancer resource #152

Merged
merged 6 commits into from
Mar 17, 2022
Merged

Conversation

coderGo93
Copy link
Contributor

  • Added parameters called sku and outbound_rules for resource azurestack_lb

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 @coderGo93 - you'll need to add outbound_rules to the docs, as well tests for both properties in addtion to the comments i've left inline

@@ -121,6 +132,16 @@ func loadBalancer() *pluginsdk.Resource {
Set: pluginsdk.HashString,
},

"outbound_rules": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be

Suggested change
"outbound_rules": {
"outbound_rule_ids": {

loadBalancer := network.LoadBalancer{
Name: pointer.FromString(id.Name),
Location: pointer.FromString(location.Normalize(d.Get("location").(string))),
Tags: tags.Expand(d.Get("tags").(map[string]interface{})),
Sku: &sku,
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we set this inline? theres nor eason to have a variable for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do that, thank you

@coderGo93 coderGo93 requested a review from katbyte February 23, 2022 22:10
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 @coderGo93 - couple comments

ForceNew: true,
ValidateFunc: validation.StringInSlice([]string{
string(network.LoadBalancerSkuNameBasic),
}, true),
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should never ignore case unless requires

Suggested change
}, true),
}, false),

@@ -121,6 +131,16 @@ func loadBalancer() *pluginsdk.Resource {
Set: pluginsdk.HashString,
},

"outbound_rules_id": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be rule ids?

Suggested change
"outbound_rules_id": {
"outbound_rule_ids": {

@coderGo93 coderGo93 requested a review from katbyte March 2, 2022 18:03
@@ -121,6 +131,16 @@ func loadBalancer() *pluginsdk.Resource {
Set: pluginsdk.HashString,
},

"outbound_rule_ids": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we ensure these are covered by a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems in order to appear the outbound rules, it will need a resource azurestack_public_ip_prefix but it is not available at the moment, I can remove the parameter outbound_rule_ids or keep it. What do you think? @katbyte

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can either open a new PR with that resource or add it to this one but that property needs to be be tested to be included here

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 the parameter because the resource azurestack_public_ip_prefix it's not supported at the current profile.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added size/XS and removed size/S labels Mar 15, 2022
@coderGo93 coderGo93 requested a review from katbyte March 15, 2022 21:14
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 @coderGo93 - LGTM 🔮

@katbyte katbyte merged commit 942f90d into hashicorp:main Mar 17, 2022
@github-actions github-actions bot added this to the v1.0.0 milestone Mar 17, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants