feat: add wordpress-microservice app into this repo #25
Conversation
5hubh49
changed the title
There was a problem hiding this comment.
- there's some commented sections that don't serve any purpose, I'd prefer to see them cleaned out.
- I'd like Diamon to review the chart and patterns, but an idea for the future improvements is to have a database be available either as part of this application (embedded) or as a separate, external, db with values we need to fill in. If the db is embedded, let's automate the password etc so folks don't need to fill it in, but can access that info later on.
applications/wordpress-microservice/templates/wordpress-deployment.yaml
Outdated
Show resolved
Hide resolved
applications/wordpress-microservice/templates/mariadb-deployment.yaml
Outdated
Show resolved
Hide resolved
| dependencies: | ||
| - name: replicated | ||
| repository: oci://registry.replicated.com/library | ||
| version: 1.0.0-beta.31 |
There was a problem hiding this comment.
Consider using Semver's version constraints to keep this up to date - https://github.com/Masterminds/semver#checking-version-constraints
| @@ -0,0 +1,17 @@ | |||
| apiVersion: kots.io/v1beta2 | |||
| kind: HelmChart | |||
There was a problem hiding this comment.
A few things to consider in this resource.
-
Keep in mind to support airgap deployments or deployments using the registry proxy that you'll need additional templating for the images to make that work. Something that you can follow up on in a future story or decide to add in this PR. I'd say generally give a look at the documenation for configuring this resource - https://docs.replicated.com/vendor/helm-native-v2-using
-
It's also a good practice to use
helmUpgradeFlagsto specify a timeout and/or the--waitflag for the charts that you install. See - https://docs.replicated.com/reference/custom-resource-helmchart-v2#helmupgradeflags
| spec: | ||
| containers: | ||
| - name: metrics-reporter | ||
| image: "{{ .Values.cronjob.image }}" |
There was a problem hiding this comment.
It's a best practice to give users the flexibility to override different parts of the image. For example, someone may need to change the registry that's used, but might want to use the default tag that you specify in the chart.
| spec: | ||
| containers: | ||
| - name: mariadb | ||
| image: mariadb:10.5 |
There was a problem hiding this comment.
It's a best practice to give users the flexibility to override different parts of the image. For example, someone may need to change the registry that's used, but might want to use the default tag that you specify in the chart.
| @@ -0,0 +1,44 @@ | |||
| {{- if .Values.database.embedded }} | |||
| apiVersion: apps/v1 | |||
| kind: Deployment | |||
There was a problem hiding this comment.
A few things to mention here:
-
Keep in mind that you don't have much templated today. It's common for people to have use cases where they might need to modify
affinity,tolerations,securityContextetc. I'm not saying you need to template out the entire Deployment API, but there's some low level things you're missing here in my opinion that you should consider adding. -
One thing to keep in mind is how will you ensure that as the values in the secret change, which you referencing with
secretKeyRefin the environment variables, how will you make sure the Deployments is rolled out to use those new changes. This doesn't happen by default in Kubernetes. See - https://helm.sh/docs/howto/charts_tips_and_tricks/#automatically-roll-deployments for how to handle this. -
Any particular reason you are templating the namespace for this resource? By default the namespace specified with
helm installwill be applied to all non cluster scoped resources, so i'd say reconsider if you need to do this at all.
| - port: {{ .Values.mariadb.service.port }} | ||
| targetPort: 3306 | ||
| selector: | ||
| app: mariadb |
There was a problem hiding this comment.
You should probably be using a helper function to reference a label here instead of hardcoding
| password: {{ .Values.database.password | default (randAlphaNum 16) | b64enc }} | ||
| root-password: {{ .Values.database.rootPassword | default (randAlphaNum 16) | b64enc }} |
There was a problem hiding this comment.
In the case that the passwords are not specified via Values, i think this will generate random passwords each time the chart is rendered which would cause some issues if you aren't finding out how to also rotate these passwords each time in the database. This pattern can work, but you'll need to look into things like using the lookup function in helm to see if you can reuse an existing password that you've generated.
| @@ -0,0 +1,37 @@ | |||
| apiVersion: apps/v1 | |||
| kind: Deployment | |||
There was a problem hiding this comment.
The same feedback i gave on the other Deployment objects applies here as well.
| targetPort: 80 | ||
| nodePort: {{ .Values.wordpress.service.nodePort }} | ||
| selector: | ||
| app: wordpress |
There was a problem hiding this comment.
Use a helper function instead of hardcoding
| maxActiveUsers: 100 # Maximum value for activeUsers | ||
|
|
||
| database: | ||
| embedded: true # Set to false to use an external database |
There was a problem hiding this comment.
I see you have the conditional for embedded vs. external but i don't see any option to provide a URI or an existing secret for connecting to that external database?
Build this wordpress app as part of a CRE task. This demo app consist of a frontend wordpress app, backend mariadb db and for custom metrics reporting I have used CronJob.
Shortcut: sc-118281