Skip to content

Feature: Ingress & Service#40

Open
Nold360 wants to merge 2 commits into
pcktdmp:masterfrom
Nold360:f_ingress
Open

Feature: Ingress & Service#40
Nold360 wants to merge 2 commits into
pcktdmp:masterfrom
Nold360:f_ingress

Conversation

@Nold360

@Nold360 Nold360 commented May 15, 2021

Copy link
Copy Markdown

Hi,

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

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

@pcktdmp

pcktdmp commented May 15, 2021

Copy link
Copy Markdown
Owner

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

@Nold360

Nold360 commented May 15, 2021

Copy link
Copy Markdown
Author

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

Here you go

@chrisjohnson00 chrisjohnson00 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread docs/index.yaml
type: application
urls:
- https://pcktdmp.github.io/charts/fahclient-2.6.5.tgz
version: 2.6.5

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread fahclient/values.yaml
hostname: chart-example.local
path: /

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

Copy link
Copy Markdown
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.

Comment thread fahclient/values.yaml
tls: false

# Ingress Annotations
annotations:

Copy link
Copy Markdown
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

pcktdmp commented Jan 20, 2022

Copy link
Copy Markdown
Owner

@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
Copy Markdown
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