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

Improve our app to prep for lambdas. #113

Merged
merged 10 commits into from
Nov 29, 2021
Merged

Conversation

joshprzybyszewski
Copy link
Owner

What broke / What you're adding

lambdas are almost ready (see #111). There's some things that'll be good to have in before that. let's do that.

How you did it

DDBid -> gamesV1
define a baseURL in the client network requests (so that we can hit a different domain).
add CORS to http request handling.
add a backdoor endpoint to see if its working or not.

How to test it and how to try to break it

@codecov
Copy link

codecov bot commented Nov 28, 2021

Codecov Report

Merging #113 (46b9123) into master (f7aa4c4) will decrease coverage by 0.10%.
The diff coverage is 30.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
- Coverage   70.47%   70.37%   -0.10%     
==========================================
  Files          86       87       +1     
  Lines        3948     3965      +17     
==========================================
+ Hits         2782     2790       +8     
- Misses        894      904      +10     
+ Partials      272      271       -1     
Impacted Files Coverage Δ
server/persistence/dynamo/utils.go 100.00% <ø> (ø)
server/server.go 57.25% <0.00%> (-1.62%) ⬇️
server/setup.go 0.00% <0.00%> (ø)
server/cors_config.go 100.00% <100.00%> (ø)
server/play/cut.go 50.00% <0.00%> (+5.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7aa4c4...46b9123. Read the comment docs.

@@ -145,10 +145,11 @@ export function useGame(): Result {
const { currentUser } = useAuth();
const { setAlert } = useAlert();
const dispatch = useDispatch();
const base = `https://lambda.hobbycribbage.com`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔮 In the future, it may be prudent to upload a settings.json as part of our SPA bundle that we can independently update without making a code change.

Then again, making a code change on a hobby project is just as fast so 🤷

Copy link
Owner Author

Choose a reason for hiding this comment

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

I agree. I don't know how to do that. What's the trick?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit more work than this (obviously), and it's been a while since I've done this, but generally:

  1. We'd have to know the name of the bucket from which the CDN distributes our bundle (CDK would give us this)
  2. We'd have to upload settings.json to that bucket, as a sibling to our index.html
  3. We'd have to handle uploading that with our deploy -- in the past, I've used a separate ts-node script and a postdeploy npm script in our CDK package to do this (notes about post* scripts)
  4. We'd also want to tell our CDN to invalidate the cache for settings.json so our clients get the updated version
  5. Ideally, we'd also want an easy way to upload something there independently of our deploy since that's the whole point of this

This smells like a separate PR to me.

@@ -21,6 +21,7 @@ export function useActiveGames(): ReturnType {
const { currentUser } = useAuth();
const { setAlert } = useAlert();
const dispatch = useDispatch();
const base = `https://lambda.hobbycribbage.com`;
Copy link
Collaborator

@cszczepaniak cszczepaniak Nov 29, 2021

Choose a reason for hiding this comment

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

At the very least, we might pull this out into a separate file so it can be shared in all the places it needs to be. Perhaps we could make a cribbageClient of some sort that's constructed with a base URL and takes relative paths for requests.

Copy link
Owner Author

Choose a reason for hiding this comment

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

💡 Right now, these are all the same because we're using lambdas for everything. Someday, we could have a "games" service, and a "recommendation" service, and a "AI hand identifier" service. In each of those, they might hit different subdomains.

Perhaps, it's best to have a gameBaseURL var that we would use for all of the current usages...

Copy link
Collaborator

@cszczepaniak cszczepaniak Nov 29, 2021

Choose a reason for hiding this comment

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

I'm fine with that. We could also say YAGNI and keep it baseURL until we need something different, but my main concern is centralizing the value of it, and the identifier is more :meh: to me

💡 🔮 Maybe it would be fun to learn about OpenAPI/Swagger and get it to generate a TypeScript SDK for us based on our games API spec

@@ -10,7 +10,7 @@ import (

const (
dbName = `cribbage`
partitionKey = `DDBid`
partitionKey = `gamesV1`
Copy link
Collaborator

Choose a reason for hiding this comment

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

📖 Is it a good idea to name our partition which stores games, players, interactions, etc. gamesV1?

Copy link
Owner Author

Choose a reason for hiding this comment

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

true. I guess it should be descriptive of what's in the value attribute field. Perhaps uuid or simply id? I like something that's "unique enough" so it's easy to correlate and search for in the code. Maybe cribbageID?

Copy link
Collaborator

Choose a reason for hiding this comment

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

cribbageID sounds good to me. I suppose this is one of the idiosyncrasies of dynamo preferring one table for storing many types of things 🤷

Comment on lines +108 to +113
router.GET(`/backdoor`, func(c *gin.Context) {
c.String(
http.StatusOK,
`The front door is open. Come on in!`,
)
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to keep this?

Copy link
Owner Author

@joshprzybyszewski joshprzybyszewski Nov 29, 2021

Choose a reason for hiding this comment

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

🤷 I like it. It's an easy way to test that the lambda is deployed. And it's occasionally fun to have easter eggs.

@joshprzybyszewski joshprzybyszewski merged commit 83cae81 into master Nov 29, 2021
@joshprzybyszewski joshprzybyszewski deleted the improveThings branch November 29, 2021 03:06
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.

2 participants