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

updated FQDN's to googleapis.com with a trailing dot #2214

Merged

Conversation

ericuldall
Copy link
Contributor

@ericuldall ericuldall commented Apr 11, 2017

** edit by @stephenplusplus **

Due to an issue brought up in #2249, we have reverted this change.


PR for Issue #2213

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 11, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c786850 on restorationmedia:issue-2213-fqdn-trailing-dot into 184f9bb on GoogleCloudPlatform:master.

@ericuldall
Copy link
Contributor Author

I caught an issue where scopes cannot have the trailing dot. I'm fixing that now.

@ericuldall
Copy link
Contributor Author

ericuldall commented Apr 11, 2017

I think I got them all, per grep -rni 'googleapis\.com\./auth' * from root of repo

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a2115be on restorationmedia:issue-2213-fqdn-trailing-dot into 184f9bb on GoogleCloudPlatform:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling cada67e on restorationmedia:issue-2213-fqdn-trailing-dot into 184f9bb on GoogleCloudPlatform:master.

Copy link
Contributor

@lukesneeringer lukesneeringer left a comment

Choose a reason for hiding this comment

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

I am saying no to this PR. I recognize that there are locations where a trailing period is appropriate (e.g. a BIND configuration file), but its general use in URLs is not one of those places.

And, in fact, when I actually try to use a URL with a trailing dot, I get redirected to the URL without it:

$ curl -I https://github.com./GoogleCloudPlatform
HTTP/1.1 301 Moved Permanently
Content-length: 0
Location: https://github.com/GoogleCloudPlatform
Connection: close

Additionally, this violates the principle of lease surprise. Even if it were a tiny benefit to write the URLs this way, the level of "huh?" caused by people who read it would be...high.

.jscsrc Outdated
@@ -16,7 +16,7 @@
],
"requireCamelCaseOrUpperCaseIdentifiers": "ignoreProperties",
"maximumLineLength": {
"value": 80,
"value": 81,

This comment was marked as spam.

@@ -66,7 +66,7 @@ function BigQuery(options) {
}

var config = {
baseUrl: 'https://www.googleapis.com/bigquery/v2',
baseUrl: 'https://www.googleapis.com./bigquery/v2',

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@lukesneeringer
Copy link
Contributor

I discussed this with our team internally and have been persuaded. I do want you to change the maximum line length back to 80, though.

@ericuldall
Copy link
Contributor Author

Sure, I can certainly move that back to 80 chars. What is the suggested style choice for the baseUrl lines that have reached 81 chars in the line?

@ericuldall
Copy link
Contributor Author

ericuldall commented Apr 13, 2017

Will something like this be acceptable?

  ¦ it('should set the default base URL', function() {
  ¦ ¦ assert.strictEqual(
  ¦ ¦ ¦ datastore.defaultBaseUrl_,
  ¦ ¦ ¦ 'datastore.googleapis.com.'
  ¦ ¦ );
  ¦ });

@stephenplusplus
Copy link
Contributor

Regarding style, whatever you think works best for the block the code is in, including renaming vars to be shorter, or:

var url = [
  'http://www.googleapis.com./...',
  'other-part'
].join('/');

Anything you come up I'm sure will be just fine, and thanks very much for asking!

Will something like this be acceptable:

Yep, that's perfect 👍

@ericuldall
Copy link
Contributor Author

Okay, changes made and rebased from upstream. Let me know if anything else needs to change.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c343a6e on restorationmedia:issue-2213-fqdn-trailing-dot into ** on GoogleCloudPlatform:master**.

@stephenplusplus stephenplusplus self-assigned this Apr 13, 2017
@stephenplusplus
Copy link
Contributor

Just assigning to myself -- running the system tests locally had a couple failures, so I'll need to comb over and make sure we've got the dots in the right spots.

@lukesneeringer
Copy link
Contributor

@stephenplusplus No need to do an entire umbrella release for this, but maybe release BigTable for @ericuldall. The rest of them can just show up when we release for other reasons.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Apr 13, 2017
@stephenplusplus
Copy link
Contributor

I stacked on a commit which removes the changes from areas that weren't working as expected and in the files that are generated-- those will have to be edited upstream.

@landrito @jmuk in the generated files, would you be interested in taking on this change?

@stephenplusplus stephenplusplus removed the cla: no This human has *not* signed the Contributor License Agreement. label Apr 13, 2017
@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Apr 14, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 773625b on restorationmedia:issue-2213-fqdn-trailing-dot into fa3e960 on GoogleCloudPlatform:master.

@callmehiphop callmehiphop merged commit 13d4ed5 into googleapis:master Apr 14, 2017
stephenplusplus added a commit to stephenplusplus/gcloud-node that referenced this pull request May 8, 2017
@stephenplusplus
Copy link
Contributor

Due to an issue brought up in #2249, we have reverted this change.

sofisl pushed a commit that referenced this pull request Oct 11, 2022
sofisl pushed a commit that referenced this pull request Oct 12, 2022
sofisl pushed a commit that referenced this pull request Oct 13, 2022
sofisl pushed a commit that referenced this pull request Nov 10, 2022
sofisl pushed a commit that referenced this pull request Nov 17, 2022
sofisl pushed a commit that referenced this pull request Jan 24, 2023
sofisl pushed a commit that referenced this pull request Jan 25, 2023
sofisl pushed a commit that referenced this pull request Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: no This human has *not* signed the Contributor License Agreement. core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants