-
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
Jupyterhub on EKS #228
Jupyterhub on EKS #228
Conversation
# Note: This can further restricted to specific required for each Add-on and your application | ||
#--------------------------------------- | ||
# Extend cluster security group rules | ||
cluster_security_group_additional_rules = { |
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.
do we need all of this opened up? can we open up only what is necessary?
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.
@rajarshighosal Let's test this for all add-ons and open only required ports in the next PR
ai-ml/jupyterhub/main.tf
Outdated
NodeGroupType = "core" | ||
} | ||
|
||
tags = { |
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 have local tags, but they aren't added here or to the other resources - should they be added/merged?
ai-ml/jupyterhub/variables.tf
Outdated
# Routable Public subnets with NAT Gateway and Internet Gateway | ||
variable "public_subnets" { | ||
description = "Public Subnets CIDRs. 62 IPs per Subnet/AZ" | ||
default = ["10.1.0.0/26", "10.1.0.64/26"] |
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.
setting a VPC CIDR and then subnet CIDRs like this could be problematic - better to calculate based off the VPC CIDR. For example, what if someone changes just the VPC CIDR and not the rest of the values?
6522044
to
fa1ad3e
Compare
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.
LGTM 👍🏼 I believe this is good to go now.
@rajarshighosal Let's address the remaining comments from Bryant on the next PR.
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.
Last few comments
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.
LGTM 🚀
What does this PR do?
🛑 Please open an issue first to discuss any significant work and flesh out details/direction - we would hate for your time to be wasted.
Consult the CONTRIBUTING guide for submitting pull-requests.
Jupyterhub on EKS
Cognito integration
Shared file storage for users
Motivation
More
website/docs
orwebsite/blog
section for this featurepre-commit run -a
with this PR. Link for installing pre-commit locallyFor Moderators
Additional Notes