-
Notifications
You must be signed in to change notification settings - Fork 16
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
base: main
Are you sure you want to change the base?
Conversation
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.
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.
@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. |
Co-authored-by: Ee Durbin <[email protected]>
Co-authored-by: Ee Durbin <[email protected]>
…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
…nfra into ComputeAtEdge_Test
fastly-compute/geoip/README.md
Outdated
@@ -0,0 +1,11 @@ | |||
# Empty Starter Kit for JavaScript |
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.
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)
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.
Definitely makes sense, I'll be sure to work this read.me over.
# 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. |
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.
We likely want to restore this behavior
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.
Agreed 👍
fastly-compute/geoip/fastly.toml
Outdated
# https://developer.fastly.com/reference/fastly-toml/ | ||
|
||
authors = ["[email protected]"] | ||
description = "Test" |
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.
suggestion: update description
fastly-compute/geoip/package.json
Outdated
"url": "git+https://github.com/fastly/compute-starter-kit-javascript-empty.git" | ||
}, | ||
"type": "module", | ||
"author": "[email protected]", |
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.
suggestion: Update the references here as well. Name, repo, etc
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.
suggestion: overall, run the js code through Prettier to get consistent style. There's a live playground here: https://prettier.io/playground/
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.
Thank you for this resource!
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.
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?)
fastly-compute/geoip/src/index.js
Outdated
} catch (error) { | ||
console.error(error); | ||
return new Response("Internal Server Error", { | ||
status: 500 | ||
}); | ||
} |
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.
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.
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.
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.
fastly-compute/geoip/src/index.js
Outdated
// If the value doesn't exist or is false, return Unauthorized | ||
if (!token) { |
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.
question: what determines if the token is supplied by the caller and is compared to the token retrieved from the config store?
fastly-compute/geoip/.secret.json
Outdated
@@ -0,0 +1,3 @@ | |||
{ | |||
"token" : "sup3rs3cr3t" |
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.
Based on the code changes in src/index.js I think the key in this should be the shared secret, no?
Using Fastly's Compute@Edge and getGeolocationForIPAddress to get Geolocation information about a given IP, and deploy automagically using Github Actions.