search

ns1 / ns1-go

Golang API client for NS1
Apache License 2.0
34 stars 59 forks source link

rest/zone: fix zone exists error catching #162

Closed costasd closed 2 years ago

costasd commented 2 years ago

NS1 api returns "invalid: FQDN already exists in the view" instead of "zone already exists" nowadays.

Let's catch this new error in our zone handling code.

costasd commented 2 years ago

Tests:

    --- PASS: TestZone/Create (0.00s)
        --- PASS: TestZone/Create/Success (0.00s)
        --- PASS: TestZone/Create/Error (0.00s)
    --- PASS: TestZone/Update (0.00s)
        --- PASS: TestZone/Update/Success (0.00s)
        --- PASS: TestZone/Update/Error (0.00s)
    --- PASS: TestZone/Delete (0.00s)
        --- PASS: TestZone/Delete/Success (0.00s)
        --- PASS: TestZone/Delete/Error (0.00s)
tlimoncelli commented 2 years ago

@Zach-Johnson Sorry to nudge but... any word on this?

Zach-Johnson commented 2 years ago

Hi folks, There are some legacy sub-systems here that are still going to return the old error I believe. Let me see if I can get some eyes on this, I think we'll need a slightly different approach here.

costasd commented 2 years ago

Hey @Zach-Johnson, thanks for the reply!

If it's easier instead of pulling folks in, me to just match both the new and the old error, I could do this, should be just another conditional in the PR.

Didn't push it already 'cause I'm not aware of the exact issue with these legacy systems, but happy to if it helps.

tlimoncelli commented 2 years ago

Checking for either message sounds good for the short term.

In the long term, I'd recommend that NS1 consider modifying the API to provide a better way to specify different types of errors. For example, prefix each error message with a tag that never changes.

For example:

The API spec could guarantee that the tag at the front will never change, but the text that follows can be updated as needed.

It would be appreciated if you could file a feature quest for this.

costasd commented 2 years ago

@Zach-Johnson I've modified the code to support both the new and the old message, so that it could potentially support these old systems as well.

Please let me know if there's anything else I should do, thanks!

meghb commented 2 years ago

Can we please merge this soon? Facing the same issue

eravin-ns1 commented 2 years ago

A fix for this has been merged in https://github.com/ns1/ns1-go/pull/167 (the code needed an additional tweak).

tshougrakpam commented 2 years ago

issue already covered