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

Get geolocation about a given IP address #138

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

cegerhardson
Copy link
Contributor

@cegerhardson cegerhardson commented Jun 2, 2023

Using Fastly's Compute@Edge and getGeolocationForIPAddress to get Geolocation information about a given IP, and deploy automagically using Github Actions.

Copy link
Contributor Author

@cegerhardson cegerhardson left a comment

Choose a reason for hiding this comment

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

Authenticated API using fastly profile create and reset the API Token via gh secret set. This comment and uncommenting from these last 2 commits serves to trigger the new Auth set up (and has such, ran successfully), and errors on the second go bc the service was already created in fastly.

@ewdurbin
Copy link
Member

ewdurbin commented Jun 8, 2023

@cegerhardson I think if you configure the service id here: https://github.com/pypi/infra/pull/138/files#diff-091f525b8dbb48737c908263259c0fbb5eb7b3488c22c76fd4d86ce631047d4bR9

it should stop blowing up.

fastly-compute/geoip/fastly.toml Outdated Show resolved Hide resolved
fastly-compute/geoip/fastly.toml Outdated Show resolved Hide resolved
cegerhardson and others added 6 commits June 8, 2023 10:48
…IpAddress

Place deploy-geoip.yaml in the correct location to be triggered

Prepare for testing, and delete old file paths.

Add project_directory value

Move project_diretory value

Trying to fix path bug.

Trying again

Just trying

Fingers-crossed

Testing for auth

Maybe?

Implement a github actions to deploy geoip
@@ -0,0 +1,11 @@
# Empty Starter Kit for JavaScript
Copy link
Member

Choose a reason for hiding this comment

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

Update this doc with some helpful information!

I'd suggest at a minimum:

  • Changing the title
  • Giving a brief description of what this service does
  • Adding some notes on how to work on it locally (npm install, commands to start the server, etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely makes sense, I'll be sure to work this read.me over.

@ewdurbin ewdurbin dismissed their stale review June 15, 2023 12:21

updates

Comment on lines +4 to +5
# branches: [main]
## Note: by removing the branches value, the workflow will run when a push is made to any branch. This is to serve for testing.
Copy link
Member

Choose a reason for hiding this comment

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

We likely want to restore this behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed 👍

fastly-compute/geoip/.secret.json Outdated Show resolved Hide resolved
# https://developer.fastly.com/reference/fastly-toml/

authors = ["[email protected]"]
description = "Test"
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: ‏update description

"url": "git+https://github.com/fastly/compute-starter-kit-javascript-empty.git"
},
"type": "module",
"author": "[email protected]",
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: ‏Update the references here as well. Name, repo, etc

fastly-compute/geoip/fastly.toml Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: ‏overall, run the js code through Prettier to get consistent style. There's a live playground here: https://prettier.io/playground/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for this resource!

Copy link
Member

@miketheman miketheman Jun 30, 2023

Choose a reason for hiding this comment

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

Might be useful to rerun prettier again.

(Also looks like per-file comments can't be resolved. FYI @github )
(Maybe that's a permissions thing though?)

Comment on lines 37 to 42
} catch (error) {
console.error(error);
return new Response("Internal Server Error", {
status: 500
});
}
Copy link
Member

Choose a reason for hiding this comment

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

question: ‏the entire function seems to be wrapped in the try/catch block.
Would Fastly respond with a 500 status if there were any errors without the try/catch? Do the function errors log to the admin console or something?

Basically, I'm asking whether we can simplify the outer behavior by letting their platform operate and return appropriate error response code is catastrophe occurs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code format was retrieved from an example on fastly's documentation, and it seems should be reworked to better match what we are trying to produce.

Comment on lines 11 to 12
// If the value doesn't exist or is false, return Unauthorized
if (!token) {
Copy link
Member

Choose a reason for hiding this comment

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

question: ‏what determines if the token is supplied by the caller and is compared to the token retrieved from the config store?

@@ -0,0 +1,3 @@
{
"token" : "sup3rs3cr3t"
Copy link
Member

Choose a reason for hiding this comment

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

Based on the code changes in src/index.js I think the key in this should be the shared secret, no?

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.

3 participants