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

Use static resource ID if "id" field is nil #638

Closed

Conversation

janik-cloudflare
Copy link
Member

This is currently the case for Secondary DNS records. In the future these records will get their own IDs, but for now, let's do this to prevent a panic when casting a nil interface{} to a string.

References: DNS-10305

This is currently the case for Secondary DNS records. In the future
these records will get their own IDs, but for now, let's do this to
prevent a panic when casting a nil `interface{}` to a string.

References: DNS-10305
@janik-cloudflare
Copy link
Member Author

... or perhaps it's fine to ask customers to set USE_STATIC_RESOURCE_IDS manually in the case of Secondary DNS for now? After all, these records are managed at some other provider, not Cloudflare. Let me know if this PR is a bad idea.

@jacobbednarz
Copy link
Member

while this may fix the generation, in reality this isn't going to work as the id field is used as an anchor/reference identifier. without it, you cannot import or manage the resource we are creating here.

@janik-cloudflare
Copy link
Member Author

Makes sense, thought there'd be a catch! Let's close this, thanks Jacob!

@janik-cloudflare
Copy link
Member Author

Importing/managing wouldn't work anyway for Secondary DNS records (at least as of now), but since this affects other resources too, let's not add a hack just for Secondary DNS. Especially since these are going to get IDs in the future :)

@jacobbednarz
Copy link
Member

sounds good! thanks

@jacobbednarz
Copy link
Member

jacobbednarz commented Nov 16, 2023

if you have a way to determine when the records are associated with secondary DNS (maybe the lack of ID is enough?), we can skip them like we do with unmanaged rulesets -

for _, r := range jsonPayload {

that can help the DX here if that is your end goal (at least for now).

@janik-cloudflare
Copy link
Member Author

I'd say it's probably fine for now since we're changing this anyway. The customer in this case was trying to fetch an entire zone, which would consist of 100% Secondary records (except perhaps one override record). Let's see what they say about my suggested work-around of setting the environment variable. Thanks for your help!

@jacobbednarz
Copy link
Member

jacobbednarz commented Nov 17, 2023

setting the environment variable will generate the records however they won't be able to do anything with them (no IDs to import or reference). they will also need to update every resource name to prevent collisions.

note: the USE_STATIC_RESOURCE_IDS is largely a CI only feature and doesn't really help anyone in the generation of resources for use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants