-
Notifications
You must be signed in to change notification settings - Fork 203
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
[SmugMug] remove usage of %fqdn% template var; add subdomain template #552
Conversation
GoDaddy does not support the %fqdn% builtin variable, which serves as a shortcut to [host.]domain https://github.com/Domain-Connect/spec/blob/master/Domain%20Connect%20Spec%20Draft.adoc#692-variable-values Instead, use "www.%domain%" as the redirect target for the domain template. Additionally use an absolute value for the CNAME host, effectively ignoring any host provided when applying the template. (see note on absolute trailing dot here: https://github.com/Domain-Connect/spec/blob/master/Domain%20Connect%20Spec%20Draft.adoc#53-template-record) The `host` field for the "REDIR301", may not do anything, as its unclear from the docs whether it is considered for a REDIR301 context, but it specifies an absolute host as well, effectively hard coding the redirect to be from %domain% > www.%domain%. In the event that host is ignored here, its not a big deal because we dont pass a host when applying so the end result will be the same. Its more for correctness of the template. Additionally, a second, subdomain specific template was created. This template will only be applied to subdomains (photos.mysite.org). There is no redirect associated with it.
Linter OK:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apprvd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO correction would be required to this PR.
"type": "REDIR301", | ||
"target": "www.%fqdn%" | ||
"target": "www.%domain%", | ||
"host": "%domain%." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not reviewing this template any earlier, but this construct does look strange.
If there is no intention to use this template with host variable and only for APEX, why to bother with the %domain%.
construct where @
would do exactly the same?
Note that you may face providers not implementing trailing dot the right way as it was quite late addition to the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@
and %domain%.
behave the same only when there is no host provided for template application. While we don't intend to provide a host, codifying the behavior in the template reduces the potential for implementation bugs and serves as a documentation of intended behavior.
"type": "REDIR301", | ||
"target": "www.%fqdn%" | ||
"target": "www.%domain%", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also should have noticed this one during the first review.
HTTP Redirect target shall be defined as an URL, so just a domain name would not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so you're saying scheme is required? ie. https://www.%domain%
. (we provide certs so hard-coding https is fine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct
GoDaddy does not support the
%fqdn%
builtin variable, which serves as a shortcut to [host.]domainhttps://github.com/Domain-Connect/spec/blob/master/Domain%20Connect%20Spec%20Draft.adoc#692-variable-values
Instead, use "www.%domain%" as the redirect target for the domain template. Additionally use an absolute value for the CNAME host, effectively ignoring any host provided when applying the template. (see note on absolute trailing dot here: https://github.com/Domain-Connect/spec/blob/master/Domain%20Connect%20Spec%20Draft.adoc#53-template-record)
The
host
field for the "REDIR301", may not do anything, as its unclear from the docs whether it is considered for a REDIR301 context (though other templates use it), but it specifies an absolute host as well, effectively hard coding the redirect to be from %domain% > www.%domain%. In the event that host is ignored here, it's not a big deal because we dont pass a host when applying so the end result will be the same. It's more for correctness of the template.Additionally, a second, subdomain specific template was created. This template will only be applied to subdomains (ie. photos.mysite.org). There is no redirect associated with it.
Other changes:
logoUrl
to SmugMug hostedsyncRedirectDomain
entries