Skip to content

Conversation

khvn26
Copy link
Member

@khvn26 khvn26 commented Jan 24, 2024

Thanks for submitting a PR! Please check the boxes below:

  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?

Changes

This adds a fully working Terraform example for deploying Flagsmith on top of Azure's managed Kubernetes and Flexible Server PostgreSQL.

How did you test this code?

This code currently works to deploy Flagsmith for one of our private cloud customers.

@khvn26 khvn26 requested a review from matthewelwell January 24, 2024 16:49
Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

Added a few comments here. Mostly very minor.

Comment on lines +27 to +28
api_domain = flagsmith.mycompany.com # Domain for the Flagsmith API
frontend_domain = api.flagsmith.mycompany.com # Domain for the Flagsmith app
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: feels like this are the wrong way around.

Suggested change
api_domain = flagsmith.mycompany.com # Domain for the Flagsmith API
frontend_domain = api.flagsmith.mycompany.com # Domain for the Flagsmith app
api_domain = api.flagsmith.mycompany.com # Domain for the Flagsmith API
frontend_domain = flagsmith.mycompany.com # Domain for the Flagsmith app


## Deployment

### Inputs
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to indicate which of these are required. For example, it feels as though the flagsmith_on_flagsmith_api_key shouldn't be required.

```diff
locals {
flagsmith_api_docker_repository = "flagsmith/flagsmith-private-cloud"
+ flagsmith_api_version = "3.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not: seems odd that we're specifying 3.0.0 here when it doesn't exist (and we have no real major plans to do so). Not a major issue just feels a little odd.

administrator_password = var.db_admin_password
zone = "1"
storage_mb = 32768
sku_name = "MO_Standard_E4ds_v4"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an input?

name = "${var.project_name}-db-server"
resource_group_name = azurerm_resource_group.default.name
location = azurerm_resource_group.default.location
version = "11"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should bump this to 15

Comment on lines +65 to +68
set {
name = "frontend.extraEnv.FLAGSMITH_ON_FLAGSMITH_API_URL"
value = "https://${var.api_domain}/api/v1/"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would love it if we could parameterise this somehow so that it's not required for people to set up flagmsith on flagsmith unless they have to?

@@ -0,0 +1,105 @@
locals {
flagsmith_api_docker_repository = "flagsmith/flagsmith-private-cloud"
flagsmith_api_version = "2.64.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's bump this to the latest - 2.96.0 I think?

@@ -0,0 +1,105 @@
locals {
flagsmith_api_docker_repository = "flagsmith/flagsmith-private-cloud"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should default to flagsmith/flagsmith-api since it's open source?

- flagsmith_api_version = "2.64.0"
}
```
2. Change the frontend version in [helm/flagsmith-values.yaml](helm/flagsmith-values.yaml):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to understand why the frontend version is set in the values.yaml whereas the api version is set as a terraform variable?


frontend:
image:
tag: 2.64.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's bump this to the latest version - 2.96.0 I think?

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.

2 participants