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

GCLOUD: display all correction messages affecting same label + type in a zone #2759

Merged
merged 1 commit into from
Jan 3, 2024
Merged

GCLOUD: display all correction messages affecting same label + type in a zone #2759

merged 1 commit into from
Jan 3, 2024

Conversation

asn-iac
Copy link
Contributor

@asn-iac asn-iac commented Jan 3, 2024

fixes #2758

Release changelog section

  • GCLOUD: display all correction messages affecting same label + type in a zone

@tlimoncelli
Copy link
Contributor

Ah! Thanks for spotting this bug!

It makes me wonder if all the providers that use IncrementalDiff will have the same problem.

(Fundamentally we need to rewrite this provider to not use the backwards compatibility mode. Using diff2 would make all that code much cleaner.)

@tlimoncelli tlimoncelli merged commit b71fd63 into StackExchange:master Jan 3, 2024
3 checks passed
@asn-iac
Copy link
Contributor Author

asn-iac commented Jan 3, 2024

I'd guess it depends on how the provider is handling the messages coming from IncrementalDiff. GCLOUD used to just copy all of the message strings from IncrementalDiff into a separate []string and print them all out as a single correction. I had actually implemented differ.ChangedGroups for the GCLOUD provider in a private fork of 2.x, which worked well, but then reverted to IncrementalDiff when I contributed #2482 in an effort to reduce the number of changes and because of the ongoing efforts with diff2 (ByRecordSet looked like it could be used for this provider, but based on the docs I'm unsure if the provider API conforms with the intent of that function). That's where this message display bug was originally introduced.
Thanks for the quick review!

@tlimoncelli
Copy link
Contributor

@asn-iac Sounds fair!

By the way... would you be interested in taking a try at rewriting it with diff2? I'd be glad to collaborate on it. For example, if ByRecordSet is insufficient, I'd be glad to fix it or provide a new By function that matches the API.

PJEilers pushed a commit to realtimeregister/dnscontrol that referenced this pull request Jan 8, 2024
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.

GCLOUD: correction message strings for multiple changes affecting same label + type are partially dropped
2 participants