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

Feature: Ingress & Service #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Nold360
Copy link

@Nold360 Nold360 commented May 15, 2021

Hi,

i implemented simple ingress and service resources & bumped the version.

Don't know how this works out with horizontal scaling enabled, tho.

@pcktdmp
Copy link
Owner

pcktdmp commented May 15, 2021

Nice! Could you also create the release tarball? Instructions are in the README :-)

@Nold360
Copy link
Author

Nold360 commented May 15, 2021

Nice! Could you also create the release tarball? Instructions are in the README :-)

Here you go

Copy link
Contributor

@chrisjohnson00 chrisjohnson00 left a comment

Choose a reason for hiding this comment

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

A couple suggestions, plus 2 things that I think must be fixed:

  1. The version number doesn't follow semver, it should be 2.6.0 next, not 2.6.5
  2. The addition of the annotations for the ingress look like it will fail if none are provided, so a default empty set should be included in the values.yaml for folks who aren't using annotations.

type: application
urls:
- https://pcktdmp.github.io/charts/fahclient-2.6.5.tgz
version: 2.6.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version: 2.6.5
version: 2.6.0

And the corresponding changes elsewhere.

@@ -0,0 +1,15 @@
apiVersion: v1
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole file should be behind a feature flag, just like the ingress. In my deployment, I don't need a service, so wouldn't want one created.

hostname: chart-example.local
path: /

# Enable TLS for Ingress e.g. letsencrypt using cert-manager
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably mention here that the secret name has an expected value, or add a new variable to specify it, for those not using cert-manager.

tls: false

# Ingress Annotations
annotations:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
annotations:
annotations: {}

IIRC, the chart will fail to deploy because the value isn't provided here for annotations.

@pcktdmp
Copy link
Owner

pcktdmp commented Jan 20, 2022

@Nold360 thank you for your contribution! I'm sorry for not following up accordingly after your last action.
@chrisjohnson00 @Nold360 Does making an ingress in general even make sense? Since you will get a response from a random pod which is running fahclient.

@chrisjohnson00
Copy link
Contributor

@Nold360 thank you for your contribution! I'm sorry for not following up accordingly after your last action. @chrisjohnson00 @Nold360 Does making an ingress in general even make sense? Since you will get a response from a random pod which is running fahclient.

I'm unclear of the use case or functionality. I generally just look at log output to get a sense for what job id was assigned and it's progress. Perhaps the web ui provides a more user friendly way to see this information?

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