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

azurerm_network_interface & azurerm_arm_loadbalancer: fixing d.Set() set errors and some additional validation #1403

Merged
merged 6 commits into from
Jun 19, 2018

Conversation

katbyte
Copy link
Collaborator

@katbyte katbyte commented Jun 15, 2018

a result of investigating #1318

@katbyte katbyte added this to the 1.8.0 milestone Jun 15, 2018
@katbyte katbyte requested a review from a team June 15, 2018 21:27
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Left some comments inline, but this is otherwise off to a good start :)

//todo, match subscription/{guid} and provider/name.name perhaps?
if matched := regexp.MustCompile(`(/subscriptions/)|(/providers/)`).Match([]byte(v)); !matched {
errors = append(errors, fmt.Errorf("azure resource ID %q should start with with '/subscriptions/{subscriptionId}' or '/providers/{resourceProviderNamespace}/'", k))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we'd be better to just call parseAzureResourceID here?

Copy link
Contributor

Choose a reason for hiding this comment

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

tenants are also a possible prefix too e.g. /tenants/000/subscriptions/000/...

_, errors := Ip4Address(tc.Ip, "test")

if len(errors) < tc.Errors {
t.Fatalf("Expected AzureResourceId to have an error for '%q'", tc.Ip)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor technically '%q' can be just %q

Required: true,
Type: schema.TypeString,
Required: true,
ValidateFunc: validate.StringNotEmpty,
Copy link
Contributor

Choose a reason for hiding this comment

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

there's a generic validation function in Core for "no default value" which may be better to use?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't know about that one! validation.NoZeroValues is perfect.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

a few minor comments, otherwise LGTM 👍

return idObj, nil
}

func composeAzureResourceID(idObj *ResourceID) (id string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see anything using this apart from the Tests, in which case can we remove this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the top level one, and kept this and the tests here on the new function

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, I meant since this is Private and it's not being used within this Package, I think this is dead code?

Type: schema.TypeString,
Optional: true,
Computed: true,
ValidateFunc: azure.ValidateResourceId,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an IP Address, not a Resource ID?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops, got copy paste happy it seems

// into a ResourceID. We make assumptions about the structure of URLs,
// which is obviously not good, but the best thing available given the
// SDK.
func ParseAzureResourceID(id string) (*ResourceID, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

are we also going to call this method from the existing method to give us a migration path whilst refactoring?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thats a good idea, now threading down to new method

@katbyte katbyte merged commit c89a74e into master Jun 19, 2018
katbyte added a commit that referenced this pull request Jun 19, 2018
@tombuildsstuff tombuildsstuff deleted the b-set_schema_errors branch June 20, 2018 06:39
@ghost
Copy link

ghost commented Mar 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
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.

2 participants