Skip to content

feat: add new altinn-app helm chart#46

Open
sduranc wants to merge 10 commits intomainfrom
altinn-app-chart
Open

feat: add new altinn-app helm chart#46
sduranc wants to merge 10 commits intomainfrom
altinn-app-chart

Conversation

@sduranc
Copy link

@sduranc sduranc commented Nov 15, 2024

Description

  • Creates a new chart for common use for Altinn apps
  • Includes the possibility to add (cron)jobs in the values if necessary
  • Can include slos and use workload identity if desired

Related Issue(s)

  • #{issue number}

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

@sduranc
Copy link
Author

sduranc commented Feb 10, 2025

I think we shouldn't add the "common" hsts-header here and just let the app default to the hsts in -n traefik, which will be there anyway, as it's deployed by the platform @bengtfredh - what do you think?

Comment on lines +56 to +72
readinessProbe:
httpGet:
path: /health
port: {{ .Values.service.internalPort }}

initialDelaySeconds: 30
failureThreshold: 3
periodSeconds: 3
timeoutSeconds: 1
livenessProbe:
httpGet:
path: /health
port: {{ .Values.service.internalPort }}
initialDelaySeconds: 3
failureThreshold: 3
periodSeconds: 10
timeoutSeconds: 2
Copy link
Member

Choose a reason for hiding this comment

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

Should we make it possible to change the liveness/readiness probes?
Also should we add a default startup probe?

@bengtfredh
Copy link
Member

I think we shouldn't add the "common" hsts-header here and just let the app default to the hsts in -n traefik, which will be there anyway, as it's deployed by the platform @bengtfredh - what do you think?

Middleware is namespaced, so apps can not use hsts defined in -n traefik. If this deployment dont add it - we need to create it another way.

sduranc and others added 2 commits February 14, 2025 12:28
Co-authored-by: Vemund Gaukstad <tjololo@users.noreply.github.com>
Co-authored-by: Vemund Gaukstad <tjololo@users.noreply.github.com>
@sduranc
Copy link
Author

sduranc commented Feb 14, 2025

I think we shouldn't add the "common" hsts-header here and just let the app default to the hsts in -n traefik, which will be there anyway, as it's deployed by the platform @bengtfredh - what do you think?

Middleware is namespaced, so apps can not use hsts defined in -n traefik. If this deployment dont add it - we need to create it another way.

Ah, interesting, didn't know that. Should be added here then.

name: {{ .Values.appName }}
labels:
{{- include ".Chart.Name.labels" . | nindent 4 }}
spec:
Copy link
Member

@bengtfredh bengtfredh May 27, 2025

Choose a reason for hiding this comment

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

add optional config for dual stack.

spec:
  ipFamilyPolicy: PreferDualStack
  ipFamilies:
  - IPv4
  - IPv6

Copy link
Member

Choose a reason for hiding this comment

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

Would this break in a non dual-stack cluster?

Copy link
Member

Choose a reason for hiding this comment

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

From what I can find. PreferDualStack is safe on non-dual-stack; just don’t use RequireDualStack unless you’re sure the cluster supports it.

You where thinking to make it default?

Copy link
Member

Choose a reason for hiding this comment

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

We made it default but optional in deployment chart:
05d3acd#diff-55f3c921ec57f475e38963114bc7afc38b746a6acc33b6641182d13be3be13c8

Copy link
Member

Choose a reason for hiding this comment

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

I assumed it would be ok when it says prefer, but was in doubt so asked.

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