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

Add FQDN endpoint identifyer #1464

Closed
wants to merge 1 commit into from
Closed

Add FQDN endpoint identifyer #1464

wants to merge 1 commit into from

Conversation

philsbln
Copy link
Contributor

Add an option to distinguish FQDNs from unqualified HostNames to address concerns from #1460

Copy link
Contributor

@mwelzl mwelzl left a comment

Choose a reason for hiding this comment

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

"ABSTAIN":

I see, this implements a comment you added to a previous PR.

Is this an improvement or not? I'm not sure - on the one hand, it makes the API bigger, but on the other, it makes the dangers of not using an FQDN somewhat more explicit. That's a bit like our decision to have an "InitiateWithSend" call as opposed to adding a "data" parameter to "Initiate".

There's a design trade-off here, and I'm undecided about what is better.

Also, I'm not sure it's a good idea to make such a change that late in the process.

@philsbln philsbln requested a review from britram January 27, 2024 20:24
Comment on lines +726 to +732
- FQDN (string containing a Fully Qualified Domain Name):

~~~
RemoteSpecifier.FQDN("example.com")
~~~

- HostName (string that gets qualified while resolving, e.g., using DNS search domains):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- FQDN (string containing a Fully Qualified Domain Name):
~~~
RemoteSpecifier.FQDN("example.com")
~~~
- HostName (string that gets qualified while resolving, e.g., using DNS search domains):
- HostName (a string, which SHOULD be a Fully Qualified Domain Name):

Comment on lines +773 to +774
Applications creating Endpoint objects using `WithHostName` should consider using fully-qualified
domain names (FQDNs) instead. Not providing an FQDN will result in the Transport Services Implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Applications creating Endpoint objects using `WithHostName` should consider using fully-qualified
domain names (FQDNs) instead. Not providing an FQDN will result in the Transport Services Implementation
Applications creating Endpoint objects using `WithHostName` SHOULD provide fully-qualified
domain names (FQDNs). Not providing an FQDN will result in the Transport Services Implementation

@mwelzl
Copy link
Contributor

mwelzl commented Jan 29, 2024

Can we close this?

I think the consensus was that it would only be ok to do with the changes suggested by @tfpauly , but then this does not really add any extra functionality since names are already specified to be FQDN's with a SHOULD.

@tfpauly
Copy link
Contributor

tfpauly commented Jan 29, 2024

I think this is fine to close

@philsbln
Copy link
Contributor Author

With the changes discussed, it does not really add any value but adds changes to the next review phase

@philsbln philsbln closed this Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants