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

update api key #2227

Merged
merged 2 commits into from
Feb 13, 2019
Merged

update api key #2227

merged 2 commits into from
Feb 13, 2019

Conversation

inigomedina
Copy link
Contributor

We need to add the new API key for Google Maps examples. This is the first update to see if it works properly. If it works, I'm going to update the rest of the examples.

We need to add the new API key for Google Maps examples. This is the first update to see if it works properly. If it works, I'm going to update the rest of examples.
@VictorVelarde
Copy link
Contributor

VictorVelarde commented Feb 13, 2019

Hi @inigomedina , thx for pushing this!

After reading the original issue (https://github.com/CartoDB/developers/issues/514), I think we are going to tackle this from RT, so I'm pinging here @rjimenezda

The main steps are probably pretty clear:
carto.js

developers

  • New PR, to deploy this provisional branch to staging and review the carto.js examples there
  • If everything is fine, merge PR in carto.js > new version release (patch?)
  • And finally merge developers PR to deploy the changes to fix this problem

If any situation arises with this, we can make the proper updates here, or talk together.

@inigomedina
Copy link
Contributor Author

Thanks a lot, @VictorVelarde. The plan sounds perfect. 🔝

@rjimenezda
Copy link
Contributor

Hi @inigomedina

I'm trying this locally, and it seems like localhost is not allowed as a host. Could you please try to authorize it?

image

This doesn't happen with the old keys, I get a quota error.

If you still have access to the old key, please make sure that all the allowed referers are the same. I can take care of updating everything.

@VictorVelarde So I should upgrade the version as well? It's 3.32 right now

@inigomedina
Copy link
Contributor Author

I'm trying this locally, and it seems like localhost is not allowed as a host. Could you please try to authorize it?

@jvillarf was the person in charge of that part on this issue. He precisely talked about this, because he restricted referrers to http|https://carto.com/developers/*.

cc @jvillarf

@jvillarf
Copy link

@inigomedina @ramiroaznar i've just disabled the referrer restriction, so you can test it locally. Would you please go ahead? (GCloud dashboar says the change can take up to 5 minutes, so be patient).

Once you have finished and we are ready for Production, please ping me again, so i can set the referrer restriction again.

Thanks.

@ramiroaznar
Copy link
Contributor

How can I test this?

@jvillarf
Copy link

Sorry @ramiroaznar i wanted to ping @rjimenezda instead of you and autocomplete played with me 😿

@rjimenezda
Copy link
Contributor

Thanks @jvillarf, I'll go ahead and change it for all. Will ping you as soon as we can block localhost again

@@ -104,12 +104,12 @@ <h1>Change the feature columns</h1>

function setMoreData() {
layer.setFeatureClickColumns(['name', 'pop_max', 'pop_min']);
document.getElementById('content').innerHTML = '';
document.getElementById('info').innerHTML = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this broken because of a wrong id?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but nothing serious, that's only used to "clear" a text. I noticed when checking all examples.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good boy scout 😄 !!

Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

LGTM. We can for that release

@rjimenezda rjimenezda merged commit e1db3a0 into master Feb 13, 2019
@rjimenezda rjimenezda deleted the inigomedina-patch-api-key branch February 13, 2019 17:57
@rjimenezda
Copy link
Contributor

rjimenezda commented Feb 13, 2019

Working in prod

https://carto.com/developers/carto-js/examples/

@inigomedina
Copy link
Contributor Author

Nice! 👏

@ramiroaznar
Copy link
Contributor

💥

@jvillarf
Copy link

d

Metrics show 200 OK already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants