-
Notifications
You must be signed in to change notification settings - Fork 123
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
KFP IRSA docs #680
KFP IRSA docs #680
Conversation
website/content/en/docs/deployment/cognito-rds-s3/guide-terraform.md
Outdated
Show resolved
Hide resolved
https://github.com/awslabs/kubeflow-manifests/pull/680/files#diff-eead88f45ea2c3efb8558080f849e39d0de7e74b2bbe8783dbba0fe1b53758a8L48 is it worth mentioning that the profile needs the irsa plugin like this |
{{< /tab >}} | ||
{{< tab header="Helm" lang="yaml" >}} | ||
make delete-kubeflow INSTALLATION_OPTION=helm DEPLOYMENT_OPTION=rds-s3 | ||
make delete-kubeflow INSTALLATION_OPTION=helm DEPLOYMENT_OPTION=rds-s3 PIPELINE_S3_CREDENTIAL_OPTION=irsa |
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 need a separate section for creating additional profiles if using IRSA because having an IAM role is a requirement now. A separate page for it might be better so we can link to it from multiple pages terraform and cognito-rds-s3 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.
Can't we just link here https://awslabs.github.io/kubeflow-manifests/docs/component-guides/profiles/ isn't that essentially what this guide is?
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.
Sounds fine to use parts it but we still need documentation indicating access to S3 bucket. It will be good to use profiles guide as a reference but have a specific sample
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 great, small changes requested
{{< /tab >}} | ||
{{< tab header="Helm" lang="yaml" >}} | ||
make delete-kubeflow INSTALLATION_OPTION=helm DEPLOYMENT_OPTION=rds-s3 | ||
make delete-kubeflow INSTALLATION_OPTION=helm DEPLOYMENT_OPTION=rds-s3 PIPELINE_S3_CREDENTIAL_OPTION=irsa |
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.
Sounds fine to use parts it but we still need documentation indicating access to S3 bucket. It will be good to use profiles guide as a reference but have a specific sample
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.
Great work on this PR!
cc @krhoyt this was the expected doc update for this feature
Which issue is resolved by this Pull Request:
Resolves #
Description of your changes:
Was requested to open against main for feedback this is a rough rough draft.
Testing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.