-
Notifications
You must be signed in to change notification settings - Fork 60
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
Migrate operator updates #133
Migrate operator updates #133
Conversation
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.
Thanks for improving the deployment experience and adjusting the docoumentation to reflect that. Definitely helps in ensuring the migration will be successful.
Left two small comments. Once addressed, they can be merged 😄
f5c40d0
to
2712e9c
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.
Functional review: ✅
The adjustment made in the makefile make the deployment experience so much better. Well done!
Code review: ✅
Nice adjustments to the documentation. Left small comments to make it a bit better.
**Undeploy Operator** | ||
|
||
``` | ||
make undeploy | ||
``` | ||
|
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 should do one of the following here:
- Warn the users that this command will also delete the namespace where the operator is deployed
- Make the command not delete the namespace
I'd go with the second option for being more safe: let's delete just what we've created 😄
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.
Agree. We should not delete the namesapce on undeploy.
## Setup | ||
|
||
Prerequisites: | ||
|
||
- [1Password Command Line Tool Installed](https://1password.com/downloads/command-line/) | ||
- [kubectl installed](https://kubernetes.io/docs/tasks/tools/install-kubectl/) | ||
- [docker installed](https://docs.docker.com/get-docker/) | ||
- [Generated a 1password-credentials.json file and issued a 1Password Connect API Token for the K8s Operator integration](https://support.1password.com/secrets-automation/) | ||
- [1Password Connect deployed to Kubernetes](https://support.1password.com/connect-deploy-kubernetes/#step-2-deploy-a-1password-connect-server). **NOTE**: If customization of the 1Password Connect deployment is not required you can skip this prerequisite. | ||
- [Generated a 1password-credentials.json file and issued a 1Password Connect API Token for the K8s Operator integration](https://developer.1password.com/docs/connect/get-started/#step-1-set-up-a-secrets-automation-workflow) |
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.
This phrasing is inconsistent compared to the others.
I'd suggest something like this:
- [Generated a 1password-credentials.json file and issued a 1Password Connect API Token for the K8s Operator integration](https://developer.1password.com/docs/connect/get-started/#step-1-set-up-a-secrets-automation-workflow) | |
- [A `1password-credentials.json` file generated and a 1Password Connect API Token issues for the K8s Operator integration](https://developer.1password.com/docs/connect/get-started/#step-1-set-up-a-secrets-automation-workflow) |
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 better🙂
- [kubectl installed](https://kubernetes.io/docs/tasks/tools/install-kubectl/) | ||
- [docker installed](https://docs.docker.com/get-docker/) |
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.
- [kubectl installed](https://kubernetes.io/docs/tasks/tools/install-kubectl/) | |
- [docker installed](https://docs.docker.com/get-docker/) | |
- [`kubectl` installed](https://kubernetes.io/docs/tasks/tools/install-kubectl/) | |
- [`docker` installed](https://docs.docker.com/get-docker/) |
This PR includes the updates in README and a couple of more major changes.
make docker-build
creates an image calledcontroller
, therefore we want to use it in theconfig/manager/manager.yaml
manifest.I tried to change the image name to some custom in Makefile but it brakes everything. Need further investigation on what needs to be done to change it. I think it's ok to go with the default one for now.
There was hardcoded
namespace
inconfig/default/kastomization.yaml
, therefore it deployed the operator only there.I adjusted
make deploy
command to use the current user's kubectl config namespace. So, users can control where they want to deploy the operator.