Updated SageMaker operator Helm chart installation guide #1254

RedbackThomson · 2020-01-21T19:48:18Z

Issue #, if available:

Description of changes:

Updated the documentation for installation of the Amazon SageMaker Operator for Kubernetes helm charts.

Testing done:

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

I have read the CONTRIBUTING doc
I used the commit message format described in CONTRIBUTING
I have used the regional endpoint when creating S3 and/or STS clients (if appropriate)
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have checked that my tests are not configured for a specific region or account (if appropriate)
I have used unique_name_from_base to create resource names in integ tests (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

RedbackThomson · 2020-01-21T19:49:13Z

@cadedaniel to also review, please.

sagemaker-bot · 2020-01-21T19:49:16Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2020-01-21T19:49:31Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2020-01-21T19:49:33Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2020-01-21T19:55:25Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

IvyBazan · 2020-01-21T23:30:11Z

doc/amazon_sagemaker_operators_for_kubernetes.rst

-the \ ``values.yaml`` file, which includes high-level parameters for the
-Chart. Replace the ARN here with the ARN for the OIDC-based role you’ve
+the \ ``rolebased/values.yaml`` file, which includes high-level parameters for the
+Chart. Replace the role Arn here with the Arn for the OIDC-based role you’ve


We use ARN when referring to the Amazon Resource Name. For reference: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition_operators.html#Conditions_ARN

Even within that document, the variable names refer to it as "Arn". I get confused as to when to use which capitalisation. I suppose it's "ARN" in documentation and "Arn" in code?

In documentation, we want to use Amazon Resource Name for the first usage, and ARN for subsequent usage. For code, the best practice is to conform to the standards being used.

sagemaker-bot · 2020-01-22T18:40:00Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2020-01-22T18:40:17Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2020-01-22T18:40:17Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2020-01-22T18:47:19Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

cadedaniel · 2020-01-22T19:09:41Z

doc/amazon_sagemaker_operators_for_kubernetes.rst

+    kubectl create namespace sagemaker-k8s-operator-system
+    helm install --namespace sagemaker-k8s-operator-system sagemaker-operator rolebased/


We should add a note that customers can pick their own namespace, but it must be specified in "system:serviceaccount:sagemaker-k8s-operator-system:sagemaker-k8s-operator-default" in the trust.json file. Otherwise, authentication won't work and customers will be confused.

Great idea. Added a note directly underneath

(optional) there are actually some fun directives you can use for formatting these kinds of things: https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/demo.html#admonitions

Is there a good way to check for syntax errors? (A make target?) I'd like to use the warning admonition

yeah, you can build the docs locally: https://github.com/aws/sagemaker-python-sdk#building-sphinx-docs

sagemaker-bot · 2020-01-23T20:40:18Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2020-01-23T20:40:31Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2020-01-23T20:40:31Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2020-01-23T20:47:18Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

IvyBazan

LGTM

RedbackThomson · 2020-01-23T22:24:16Z

@laurenyu

laurenyu · 2020-01-23T22:30:41Z

doc/amazon_sagemaker_operators_for_kubernetes.rst

+    kubectl create namespace sagemaker-k8s-operator-system
+    helm install --namespace sagemaker-k8s-operator-system sagemaker-operator rolebased/


(optional) there are actually some fun directives you can use for formatting these kinds of things: https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/demo.html#admonitions

doc/amazon_sagemaker_operators_for_kubernetes.rst

sagemaker-bot · 2020-01-23T22:52:24Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2020-01-23T22:52:32Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2020-01-23T22:53:03Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2020-01-23T22:58:26Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

RedbackThomson · 2020-01-23T23:11:38Z

Awesome. Ready to merge @laurenyu

Updated helm chart installation guide

83607c8

RedbackThomson requested a review from IvyBazan January 21, 2020 19:48

IvyBazan suggested changes Jan 21, 2020

View reviewed changes

Replaced Arn with ARN

6bd5761

cadedaniel reviewed Jan 22, 2020

View reviewed changes

Added note regarding namespacing

325525d

cadedaniel previously approved these changes Jan 23, 2020

View reviewed changes

IvyBazan previously approved these changes Jan 23, 2020

View reviewed changes

laurenyu reviewed Jan 23, 2020

View reviewed changes

Added sphinx admonition warning

602985f

RedbackThomson dismissed stale reviews from IvyBazan and cadedaniel via 602985f January 23, 2020 22:51

laurenyu approved these changes Jan 23, 2020

View reviewed changes

RedbackThomson mentioned this pull request Jan 23, 2020

Polish Helm charts aws/amazon-sagemaker-operator-for-k8s#76

Merged

laurenyu merged commit 63cccdf into aws:master Jan 23, 2020

RedbackThomson deleted the refactor_operator_helm_charts branch January 23, 2020 23:12

		kubectl create namespace sagemaker-k8s-operator-system
		helm install --namespace sagemaker-k8s-operator-system sagemaker-operator rolebased/

Updated SageMaker operator Helm chart installation guide #1254

Updated SageMaker operator Helm chart installation guide #1254

Uh oh!

Conversation

RedbackThomson commented Jan 21, 2020

Merge Checklist

General

Tests

Uh oh!

RedbackThomson commented Jan 21, 2020

Uh oh!

sagemaker-bot commented Jan 21, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 21, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 21, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 21, 2020

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Jan 22, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 22, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 22, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 22, 2020

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Jan 23, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 23, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 23, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 23, 2020

AWS CodeBuild CI Report

Uh oh!

IvyBazan left a comment

Choose a reason for hiding this comment

Uh oh!

RedbackThomson commented Jan 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sagemaker-bot commented Jan 23, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 23, 2020

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 23, 2020

AWS CodeBuild CI Report

Uh oh!