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.

Sign up for GitHub

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

Jump to bottom

Fix reconciler TXT record handling #465

Merged
6 commits merged into from Mar 28, 2023
Merged

Fix reconciler TXT record handling #465

6 commits merged into from Mar 28, 2023

Conversation

ghost
Copy link

@ghost ghost commented Mar 28, 2023

Closes #462

@ghost ghost self-assigned this Mar 28, 2023
humphd
humphd requested changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a few unit tests for these 2 utility functions so we don't waste time debugging this on staging.

@ghost
Copy link
Author

ghost commented Mar 28, 2023

@humphd Added!

@ghost ghost requested a review from humphd March 28, 2023 18:37
humphd
humphd previously approved these changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thank you. The only thing this doesn't do is the 4,000 limit:

The maximum length of a value in a TXT record is 4,000 characters.

However, I'm OK if we leave that out.

app/queues/reconciler/route53Utils.server.ts Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Mar 28, 2023

Excellent, thank you. The only thing this doesn't do is the 4,000 limit:

The maximum length of a value in a TXT record is 4,000 characters.

However, I'm OK if we leave that out.

Actually, I don't think our db column holds that amount of info. Even if it did, we should inhibit the insert IMO

@ghost ghost requested a review from humphd March 28, 2023 21:44
@ghost ghost merged commit 3c16e38 into DevelopingSpace:main Mar 28, 2023
@ghost ghost deleted the fix_reconciler_txt branch March 28, 2023 22:49
Genne23v pushed a commit to Genne23v/starchart that referenced this pull request Mar 29, 2023
@Genne23v
Fix reconciler TXT record handling (DevelopingSpace#465)
9e4f226 
* fix: Reconciler TXT record handling

* fix: typo

* feat: add tests, fix quotation mark handling

* fix: backslash handling

* fix

* fix: comment
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SerpentBytes SerpentBytes Awaiting requested review from SerpentBytes

@Genne23v Genne23v Awaiting requested review from Genne23v

@sfrunza13 sfrunza13 Awaiting requested review from sfrunza13

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TXT record values must be wrapped in "...", split at 255 chars, etc.
3 participants
@humphd @Eakam1007 @denke8