Fix import for compute_route #565
Conversation
google/field_helpers.go
Outdated
| // | ||
| // If the project is not specified, it first tries to get the project from the `projectSchemaField` and then fallback on the default project. | ||
| // If the zone is not specified, it takes the value of `zoneSchemaField`. | ||
|
|
google/field_helpers.go
Outdated
| // If the project is not specified, it first tries to get the project from the `projectSchemaField` and then fallback on the default project. | ||
| // If the zone is not specified, it takes the value of `zoneSchemaField`. | ||
|
|
||
| func parseZonalFieldValue(resourceType, fieldValue, projectSchemaField, zoneSchemaField string, d TerraformResourceData, config *Config, isEmptyValid bool) (*ZonalFieldValue, error) { |
There was a problem hiding this comment.
This function has 7 arguments. Is it doing too much? You might consider breaking it down into smaller functions or somehow grouping related attributes together. Up to you if you think that's simpler
There was a problem hiding this comment.
Then entry point would still require all the parameters. The logic using those parameters is extracted away in method like getProjectFromSchema.
There was a problem hiding this comment.
Well, not exactly. Some of these parameters aren't "request-level" (e.g. they don't change inside the specific resource's code) and could be extracted into a more general constructor. You could imagine something like
// Defined at top of resource
var zoneParser := ZoneParser{ResourceType: "networks", IsEmptyValid: false}
func myResourceCreate(d *schema.ResourceData, meta interface{}) {
// a bunch of stuff happens here
// [...]
zoneParser.Parse(myFieldValue, "foo_field", "project", d, config)
}Similar to how a regex's pattern is defined globally ahead of where it's actually used. But it's up to you; it may be this representation is uglier.
| globalLinkTemplate = "projects/%s/global/%s/%s" | ||
| globalLinkBasePattern = "projects/(.+)/global/%s/(.+)" | ||
| globalLinkTemplate = "projects/%s/global/%s/%s" | ||
| globalLinkBasePattern = "projects/(.+)/global/%s/(.+)" |
There was a problem hiding this comment.
I still don't enjoy seeing a mixture of regexps and formatting patterns (when reading this it's not obvious to me whether the regexp is matched before or after the print). However if this ends up reducing the amount of code needed (which I'm guessing it does given the large number of constants) then I'll defer to your judgement
Alternatively, consider inlining these (especially if they're only used once). Then the formatting/regexp matching is much more explicit
There was a problem hiding this comment.
*Template is used in more than one place.
*BasePattern is used in only one place but I like to have both next to each other. It is easy to catch a mistake.
google/field_helpers.go
Outdated
| } | ||
|
|
||
| r = regexp.MustCompile(fmt.Sprintf(zonalPartialLinkBasePattern, resourceType)) | ||
| if r.MatchString(fieldValue) { |
There was a problem hiding this comment.
You might be able to collapse this and the following line via:
if parts := r.FindStringSubmatch(fieldValue); parts != nil {
// stuff happens
}
rosbo
force-pushed
the
compute_route_network_field
branch
from
5554bd6 to
d99baf8
Compare
* Reorder fields in schema for style consistency * Add reusable ZonalFieldValue * Fix import and read state from API for compute route * Generate network link without calling the API
* Reorder fields in schema for style consistency * Add reusable ZonalFieldValue * Fix import and read state from API for compute route * Generate network link without calling the API
) Signed-off-by: Modular Magician <[email protected]>
|
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
locked and limited conversation to collaborators
Fixes one resource for #431