-
Notifications
You must be signed in to change notification settings - Fork 173
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
IPFS Pool Cache #4
Conversation
bin/stacks/routing-caching-stack.ts
Outdated
@@ -37,6 +47,13 @@ export class RoutingCachingStack extends cdk.NestedStack { | |||
], | |||
}); | |||
|
|||
lambdaRole.addToPolicy( | |||
new PolicyStatement({ | |||
resources: [route53Arn!], |
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.
Does it still deploy if route53Arn is undefined? I think IAM does some basic validation of policies during deployment so I would expect the deployment to fail?
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.
Hm, I'm not sure.. should we make it a required variable?
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.
You can remove it from your .env
and do a test deploy to see.
If it is required, you can just put this in an if (stage == Stage.beta)
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.
Ok will do!
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.
Yeah, we'd need to require the route53 arn so I just wrapped it in an if statement
lib/cron/cache-pools-ipfs.ts
Outdated
|
||
// init route53 with credentials | ||
let data; | ||
var route53 = new Route53(); |
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.
const
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.
changed to it let route53
since we redefine it below with the credentials
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.
Looks good!
checkpointing the pool cache deploy on ipfs