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

initial info for contributions #21

Merged
merged 3 commits into from
Feb 24, 2020

Conversation

mavimo
Copy link
Member

@mavimo mavimo commented Dec 29, 2019

NB: should be merged AFTER #20

  • Include MetalLB in order to be able to test ingress without any load balancer service (typically they are expensive 💸)
  • Use port forward to expose ingress (in order to be able to test using Let's encrypt)
  • Some minor fix on missing port in internal service
  • Added DOCS

FYI my next steps will be to add pebble in order to make easy test on local env the TLS part, but at the moment my focus is on make the TLS part work :)

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

This is a great enhancement, thanks!

Just a couple of nits/typos.

I am not well-versed in the metal/k8s stuff here below so I'm going to leave it to @dannav if he wants to give it a once-over in his spare time, otherwise I'm OK with merging this (after my suggestions) if you feel good about it.

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
@mavimo mavimo requested a review from mholt December 29, 2019 20:33
@dannav dannav self-requested a review December 29, 2019 21:36
Copy link
Contributor

@dannav dannav left a comment

Choose a reason for hiding this comment

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

Thanks for this, this is awesome! Just a couple of things I think we should explicitly note as optional but maybe be helpful depending on your setup.

- [skaffold](https://skaffold.dev/) (to improve development experience)
- [Docker HUB](https://hub.docker.com) account (to store your docker images)

## Setup a development cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine but should probably make it explicit that it is optional. Kubernetes comes with docker for Mac so I don’t want someone getting confused and setting up kind if they don’t need to 😛

Copy link
Member Author

@mavimo mavimo Dec 30, 2019

Choose a reason for hiding this comment

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

@dannav It's fine for you if I add a note like the one reported below?

OPTIONAL: this is needed only if you don't have a kubernetes cluster to use for development purpose (like Docker Desktop)

Copy link
Member

Choose a reason for hiding this comment

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

@mavimo That looks good to me, sure -- please go for it!

@@ -0,0 +1,307 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with this file. I would probably put it in an opt folder and link to this manifest

Copy link
Member Author

Choose a reason for hiding this comment

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

This is required to be able to use a service as LoadBalancer, I'm not sure that using an external load balancer should be useful for development purpose (and AFAIK Docker desktop do not support LoadBalancer service out of the box).
Do not enable it by default will require extra steps to contributors and only few of them will need to hack the default config.. @dannav WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, you're correct if you want to use a lb :-P. But you can do this with ingress if you want to test locally as well.

Here's an example w/ minikube and nginx: https://kubernetes.io/docs/tasks/access-application-cluster/ingress-minikube/

I guess the point I'm trying to get across is that unfortunately there are a million ways to do these things :-(. If we want to show one way to do it that's fine. But if we do, we should be explicit that it is optional unless there's a really good reason why it should be done a specific way.

Copy link
Contributor

@dannav dannav left a comment

Choose a reason for hiding this comment

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

I feel like I'm nitpicking now, ultimately I think this is fine to merge unless we want to be really explicit with the docs. Regardless when this is released a lot more docs are going to have to be created and we can always come back and clean this all up.

@mholt mholt merged commit 9155435 into caddyserver:master Feb 24, 2020
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.

3 participants