-
Couldn't load subscription status.
- Fork 255
changed proposed items #16380
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
base: master
Are you sure you want to change the base?
changed proposed items #16380
Conversation
Review of PR #16380Thanks for contributing to fix the External Secrets Operator installation! I've reviewed the changes and have some observations: Technical accuracy ✓The Helm command syntax is correct and the parameters are valid:
Concerns1. Version pinning to 0.10.4 (line 48) Version 0.10.4 appears to be quite old. External Secrets Operator has had many releases since then. Pinning to an older version in a tutorial could:
Suggestion:
2. Missing context for readers The tutorial doesn't explain why Testing recommendationSince this changes installation instructions in a tutorial, please verify:
Mention me (@claude) if you'd like additional reviews or help addressing these concerns! |
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.
Where does the version number (0.10.0) come from?
Can we include a paragraph about finding the correct version number to use, then update the code sample with a placeholder version number instead?
--version <VERSION_NUMBER>
That means when the version number changes in 6 months we're not still instructing users to install an old version.
|
Your site preview for commit fd61f96 is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-16380-fd61f967.s3-website.us-west-2.amazonaws.com. |
|
I executed the commands, k8s objects with a colleague. Updated the sample code for namespaces |
|
Your site preview for commit 43c2f1b is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-16380-43c2f1b9.s3-website.us-west-2.amazonaws.com. |
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.
A couple of small comments before this can be merged.
| #### Create ClusterSecretStore | ||
|
|
||
| Now you can create a [ClusterSecretStore](https://external-secrets.io/main/api/clustersecretstore/) resource that will tell External Secrets Operator to use Pulumi ESC as a secret provider. | ||
| You can create a [ClusterSecretStore](https://external-secrets.io/main/api/clustersecretstore/) resource that will tell External Secrets Operator to use Pulumi ESC as a secret provider. |
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.
Rearrange this so the reader reads their instructions first in case they aren't paying close attention.
Something like:
You can create a SecretStore resource that will tell the External Secrets Operator to use Pulumi ESC as a secret provider for this namespace. If you want to increase the scope of the whole cluster you can use a ClusterSecretStore instead.
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 the section, accordingly :)
| - secretKey: esc-secret | ||
| remoteRef: | ||
| key: hello | ||
| - secretKey: esc-secret |
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 you please lint this yaml? There has been extra indentation added, so check it's still valid.
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.
woow, good catch. The error was due to copy paste issue. Updated
latest master branch is merged to local branch. It will be easy for merging onto origin master
|
Your site preview for commit 20e76bf is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-16380-20e76bf5.s3-website.us-west-2.amazonaws.com. |
|
Your site preview for commit edcb694 is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-16380-edcb6948.s3-website.us-west-2.amazonaws.com. |
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.
Just a couple of grammar fixes
| You can create a [SecretStore](https://external-secrets.io/main/api/secretstore/) resource to notify External Secrets Operator to use Pulumi ESC as a secret provider for a specific namespace. | ||
|
|
||
| If you want to limit the access by namespace, you can create a [SecretStore](https://external-secrets.io/main/api/secretstore/) resource instead, which is scoped to a single namespace. | ||
| However if you want to expand the scope to entire cluster, you can use a [ClusterSecretStore](https://external-secrets.io/main/api/clustersecretstore/) resource, instead. |
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.
Grammar fixes:
However, if you want to expand the scope to the entire cluster you can use a ClusterSecretStore resource instead.
| #### Create ClusterSecretStore | ||
|
|
||
| Now you can create a [ClusterSecretStore](https://external-secrets.io/main/api/clustersecretstore/) resource that will tell External Secrets Operator to use Pulumi ESC as a secret provider. | ||
| You can create a [SecretStore](https://external-secrets.io/main/api/secretstore/) resource to notify External Secrets Operator to use Pulumi ESC as a secret provider for a specific namespace. |
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.
Grammar fixes:
You can create a SecretStore resource to notify the External Secrets Operator to use Pulumi ESC as a secret provider for a specific namespace.
| Please replace `${PULUMI_ORG}`, `${ESC_PROJECT}`, `${ESC_ENV}` with your Pulumi organization, project, and environment names. | ||
| Please replace `${PULUMI_ORG_NAME}`, `${ESC_PROJECT_NAME}`, `${ESC_ENV_NAME}` with your Pulumi organization, project, and environment names. | ||
|
|
||
| For demo purposes, we assume that we already have an [ESC environment](/docs/esc/get-started/create-environment/) `my-org/my-project/my-env` with a secret `my-secret` that we want to manage using External Secrets Operator. |
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.
Grammar fix:
using the External Secrets Operator.
|
Your site preview for commit 9194a21 is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-16380-9194a217.s3-website.us-west-2.amazonaws.com. |
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.
Almost there!
| You can create a [SecretStore](https://external-secrets.io/main/api/secretstore/) resource to notify the External Secrets Operator to use Pulumi ESC as a secret provider for a specific namespace. | ||
|
|
||
| However if you want to expand the scope to entire cluster, you can use a [ClusterSecretStore](https://external-secrets.io/main/api/clustersecretstore/) resource, instead. | ||
| However, if you want to expand the scope to entire cluster, you can use a [ClusterSecretStore](https://external-secrets.io/main/api/clustersecretstore/) resource instead. |
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.
One last comma to remove 😁
scope to entire cluster, you can use a
scope to the entire cluster you can use a
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.
And a the to add.
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.
me thinks, "scope to entire cluster, " is correct, because you need a short pause there ? But i can remove it, once you confirm the removal.
The to be added for "ClusterSecretStore"? we have been using article "a" for ClusterSecretStore. ? we have to replace all "a" s to "the" ? please confirm
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.
If you've been using a then use an entire cluster, sounds good.
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.
And you've convinced me about the comma 😁 Keep the comma.
|
Your site preview for commit 730a4a7 is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-16380-730a4a70.s3-website.us-west-2.amazonaws.com. |
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
Proposed changes
Installing ESO Operator via Helm on k8s, needed exact Chart Version and to install all CRDs used. So fixed.
Fix: #16379
Unreleased product version (optional)
Related issues (optional)