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

Ingress configuration in Helm chart #7

Merged
merged 3 commits into from
Aug 14, 2024
Merged

Ingress configuration in Helm chart #7

merged 3 commits into from
Aug 14, 2024

Conversation

assumptionsandg
Copy link
Contributor

No description provided.

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

tls:
- hosts:
- {{ .Values.ingress.host }}
secretName: {{ include "coral-credits.ingress.tls.secretName" . }}
Copy link
Contributor

@scrungus scrungus Jul 25, 2024

Choose a reason for hiding this comment

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

you need to define this in helpers.tpl (and allow override from values.yaml)

@scrungus
Copy link
Contributor

It would be nice for the functional test to reflect this. I guess that would involve adding an nginx onto the kind cluster

Comment on lines 5 to 6
annotations:
{{ .Values.ingress.annotations }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you might need something like:

{{- with .Values.ingress.annotions }}
  annotations: {{ toYaml . | nindent 4 }}
  {{- end }}

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you adapt the functional test as well to install an nginx ingress controller and test this endpoint instead of the current port forwarding?

You should be able to just add a helm command that replicates the functionality of azimuth-ops:
https://github.com/stackhpc/ansible-collection-azimuth-ops/blob/500e98bc4ff8c93e8a75207f9908e483a737c866/roles/ingress_nginx/tasks/main.yml#L3-L14

@scrungus
Copy link
Contributor

scrungus commented Aug 2, 2024

* Ingress.extensions "coral-credits" is invalid: spec.tls[0].hosts[0]: Invalid value: "": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

I think this will get fixed when you configure nginx and inject values in the functional test

@scrungus
Copy link
Contributor

scrungus commented Aug 5, 2024

@assumptionsandg this may be useful to look at https://kind.sigs.k8s.io/docs/user/ingress/.

also I normally use https://github.com/lhotari/action-upterm for debugging issues in actions

- host: {{ .Values.ingress.host }}
http:
paths:
- path: {{ .Values.ingress.path }}
Copy link
Member

Choose a reason for hiding this comment

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

Lets hard code this to "/" I think?

# The hostname to use for the portal
host:
# The path for the coral credits API
path:
Copy link
Member

Choose a reason for hiding this comment

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

Lets remove the path config for now, I think, keep it simple.

@@ -104,6 +123,7 @@ replicaCount: 1

# Service details for the api
service:
name: coral-credits
Copy link
Member

Choose a reason for hiding this comment

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

can we not call this http?

@scrungus
Copy link
Contributor

LGTM

@scrungus scrungus merged commit e71b4fc into main Aug 14, 2024
5 checks passed
@scrungus scrungus deleted the ingress branch August 14, 2024 16:04
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