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

NEW RECORD TYPE: HTTPS & SVCB #2919

Merged
merged 6 commits into from
May 1, 2024
Merged

NEW RECORD TYPE: HTTPS & SVCB #2919

merged 6 commits into from
May 1, 2024

Conversation

fritterhoff
Copy link
Contributor

No description provided.

@fritterhoff
Copy link
Contributor Author

That should fix #1660 if I complete all required additions and tests... :)

@fritterhoff
Copy link
Contributor Author

@tlimoncelli I'm a little bit unhappy about the SVCB params parsing. Any advice/hint how I can improve that and avoid the code duplication?

@tlimoncelli
Copy link
Contributor

@tlimoncelli I'm a little bit unhappy about the SVCB params parsing. Any advice/hint how I can improve that and avoid the code duplication?

How about letting miekg/dns do the parsing like in SetTargetLOCString() ?

@fritterhoff
Copy link
Contributor Author

Thanks for the hint. I already changed it!

@tlimoncelli
Copy link
Contributor

Looks good so far! When the PR is no longer marked draft, I'll do a deeper review.

One note: Please add integration tests that (1) create a record, (2) change each field one at a time.

Thanks!

@fritterhoff
Copy link
Contributor Author

Will hopefully complete the changes tomorrow and extend also the docs.

@fritterhoff fritterhoff marked this pull request as ready for review April 29, 2024 12:23
@fritterhoff
Copy link
Contributor Author

Prop. I missed some places (again) ... from my point of view the most changes should be done.

cafferata
cafferata suggested changes Apr 29, 2024
View reviewed changes
Copy link
Collaborator

@cafferata cafferata left a comment

Choose a reason for hiding this comment

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

This GitHub pull request looks great! I only have some minor feedback. ☺️

build/generate/featureMatrix.go Outdated Show resolved Hide resolved
build/generate/featureMatrix.go Outdated Show resolved Hide resolved
documentation/language-reference/domain-modifiers/HTTPS.md Show resolved Hide resolved
documentation/language-reference/domain-modifiers/SVCB.md Show resolved Hide resolved
cafferata
cafferata reviewed Apr 29, 2024
View reviewed changes
providers/bind/bindProvider.go
providers.CanUseLOC: providers.Can(),
providers.CanUseNAPTR: providers.Can(),
providers.CanUsePTR: providers.Can(),
providers.CanUseSOA: providers.Can(),
providers.CanUseSRV: providers.Can(),
providers.CanUseSSHFP: providers.Can(),
providers.CanUseSVCB: providers.Can(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

@tlimoncelli, would it be an idea to call on all provider maintainers and ask them if they would like to test these two capabilities and determine whether these capabilities can be used for their provider?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Obviously outside the scope of this GitHub pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can at least confirm it is working also for Cloudflare.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, once this is merged please post to the mailing list and ask maintainers to add support.

@fritterhoff
Copy link
Contributor Author

This GitHub pull request looks great! I only have some minor feedback. ☺️

Thanks for the feedback. Fixed the issues in the docs.

tlimoncelli
tlimoncelli reviewed May 1, 2024
View reviewed changes
Copy link
Contributor

@tlimoncelli tlimoncelli left a comment

Choose a reason for hiding this comment

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

Mostly just some cosmetic changes.

One more thing... please reformat helpers.js:

npm install prettier
node_modules/.bin/prettier --write pkg/js/helpers.js

(Please do NOT check in the new package-lock.json file)

documentation/language-reference/domain-modifiers/HTTPS.md Outdated Show resolved Hide resolved
documentation/language-reference/domain-modifiers/SVCB.md Outdated Show resolved Hide resolved
integrationTest/integration_test.go Outdated Show resolved Hide resolved
integrationTest/integration_test.go Outdated Show resolved Hide resolved
models/t_svcb.go Show resolved Hide resolved
providers/bind/bindProvider.go
providers.CanUseLOC: providers.Can(),
providers.CanUseNAPTR: providers.Can(),
providers.CanUsePTR: providers.Can(),
providers.CanUseSOA: providers.Can(),
providers.CanUseSRV: providers.Can(),
providers.CanUseSSHFP: providers.Can(),
providers.CanUseSVCB: providers.Can(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, once this is merged please post to the mailing list and ask maintainers to add support.

@tlimoncelli tlimoncelli merged commit 3a9b413 into StackExchange:main May 1, 2024
5 of 6 checks passed
@fritterhoff fritterhoff deleted the https-svcb-record branch May 1, 2024 15:38
@christianbur christianbur mentioned this pull request May 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tlimoncelli tlimoncelli tlimoncelli left review comments

@cafferata cafferata cafferata approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@fritterhoff @tlimoncelli @cafferata