Provide informative error message for policy path #1495
Conversation
annakhm
changed the title
nsxt/policy_utils.go
Outdated
| } | ||
| if errors.Is(err, ErrNotAPolicyPath) { | ||
| if !isValidResourceID(d.Id()) { | ||
| return rd, fmt.Errorf("Invalid policy path or id %s; expected path format: %s", d.Id(), pathExample) |
There was a problem hiding this comment.
Do you think this error message makes sense in the case we fail to validate the resource id? The user did not specify a path.
There was a problem hiding this comment.
There is little sense to give an example of id.. but I rephrased it to avoid confusion, does it look better now?
annakhm
force-pushed
the
improve-import-error
branch
from
09e4786 to
95c6ee9
Compare
| @@ -82,7 +82,7 @@ func resourceNsxtVpcStaticRoutes() *schema.Resource { | |||
| Update: resourceNsxtVpcStaticRoutesUpdate, | |||
| Delete: resourceNsxtVpcStaticRoutesDelete, | |||
| Importer: &schema.ResourceImporter{ | |||
| State: nsxtVPCPathResourceImporter, | |||
| State: getVpcPathResourceImporter("/orgs/[org]/projects/[project]/vpcs/[vpc]/static-routes/[route]"), | |||
There was a problem hiding this comment.
Having these long constant strings within the function call is quite terrible.
I wouldn't ask you to change all of those as it's a pain but maybe it's worthwhile to embed these in the metadata struct somehow or use a constant (especially as we plan to generate this as well).
There was a problem hiding this comment.
My personal preference is how the code is written today, I would argue its more readable this way, since you don't need to scroll up to see the example. However, I'm fine with rewriting this with a constant. I think we should have same approach for current PR and future generated code.
There was a problem hiding this comment.
I think that generated code is not supposed to be readable. It will be good to have constants, but if it takes time to implement that, there are worse thing that a long literal - like 500+ files in the same directory :)
|
LGTM, other than the comment above. |
annakhm
force-pushed
the
improve-import-error
branch
from
95c6ee9 to
7ed5df5
Compare
|
/test-all |
|
@annakhm please rebase this so tests can run |
annakhm
force-pushed
the
improve-import-error
branch
from
7ed5df5 to
1ac1e3d
Compare
|
/test-all |
| func resourceNsxtPolicyTransportZone() *schema.Resource { | ||
| return &schema.Resource{ | ||
| Create: resourceNsxtPolicyTransportZoneCreate, | ||
| Read: resourceNsxtPolicyTransportZoneRead, | ||
| Update: resourceNsxtPolicyTransportZoneUpdate, | ||
| Delete: resourceNsxtPolicyTransportZoneDelete, | ||
| Importer: &schema.ResourceImporter{ | ||
| State: resourceNsxtPolicyTransportZoneImporter, | ||
| State: getPolicyPathOrIDResourceImporter(transportZonePathSample), |
There was a problem hiding this comment.
This importer won't work here, need that one which gathers site_id
There was a problem hiding this comment.
correct, my bad! thanks Kobi
annakhm
force-pushed
the
improve-import-error
branch
2 times, most recently
from
9b68d50 to
5df4530
Compare
|
/test-all |
Whenever bad path is provided for import, we need to communicate what the expected format is. Signed-off-by: Anna Khmelnitsky <[email protected]>
Signed-off-by: Anna Khmelnitsky <[email protected]>
annakhm
force-pushed
the
improve-import-error
branch
from
5df4530 to
1131e90
Compare
|
/test-all |
Whenever bad path is provided for import, we need to communicate what the expected format is.