Skip to content
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

doc(Development): Remove cert-manager deployment #735

Conversation

thcdrt
Copy link
Collaborator

@thcdrt thcdrt commented Aug 5, 2021

Hello,

I think we can remove this cert manager command because it causes issues when running the run Makefile target that tries to launch cert-manager too but on a more recent version. I think we should let the run target handle this.

Signed-off-by: Thomas Coudert thomas.coudert@ovhcloud.com

Signed-off-by: Thomas Coudert <thomas.coudert@ovhcloud.com>
@thcdrt thcdrt force-pushed the remove-cert-manager-deployment-from-development-doc branch from 425ff79 to febedf0 Compare August 5, 2021 16:52
Comment on lines -35 to -40
```bash
helm install cert-manager \
--namespace cert-manager \
--version v0.12.0 \
jetstack/cert-manager
```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to replace it with kubectl apply -f https://github.com/jetstack/cert-manager/releases/latest/download/cert-manager.yaml

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's really relevant to do it here as it's already started in Makefile

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure if this area is worth modifying

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thcdrt

If want to let the developer use makefile to prepare the env, you can delete the section. But it seems we should explicitly mention the developer can use makefile to install prerequisite like cert-manager here. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not, we could redirect to the certmanager Makefile target, but in this case to be consistent we should talk of nginx ingress, postgresql-operator and redis-operator too, don't you think ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#713

Can we use helm to solve the problem of dependent installation?

Copy link
Collaborator

@bitsf bitsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, make install-dependencies would also install a cert-manager. this step in doc is useless.

@steven-zou
Copy link
Collaborator

@thcdrt @bitsf @cndoit18

So, does that mean we agree to merge this PR?

@steven-zou steven-zou merged commit 53ca0ac into goharbor:master Aug 12, 2021
@cndoit18
Copy link
Collaborator

@thcdrt @bitsf @cndoit18

So, does that mean we agree to merge this PR?

I agree with bitsf's comment

cndoit18 pushed a commit to cndoit18/harbor-operator that referenced this pull request Aug 13, 2021
Signed-off-by: Thomas Coudert <thomas.coudert@ovhcloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants