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

Porting Single server to Flexible server #394

Merged
merged 6 commits into from Mar 10, 2023
Merged

Conversation

pamelafox
Copy link
Member

This PR replaces PostgreSQL Single Server with Flexible Server.

A few open questions for you:

  • There's a third tier called "Memory-Optimized", should I make that an option? Is the goal to represent every tier? (I notice we only offer the cheapest version of each tier, so I assume complete comprehensiveness is not the goal)
  • I enabled zone-redundant high availability in the Bicep in order to match the 99,99% SLA, but that will affect the costs. I don't use that option in my standard samples, I use "Disabled". That lowers the SLA however. What's your preference?
  • Bicep warns about the fact that its outputting a password, but I guess you are okay with that, as you need the password for the app configuration, right? AZD got around that a different way, by inventing their own random password generator and approach.

Here are all the Bicep errors output by me deploying app service + PG, for the record:

A new Bicep release is available: v0.14.85. Upgrade now by running "az bicep upgrade".
/Users/pamelafox/Downloads/bicep 3/modules/postgresql/postgresql.bicep(71,1) : Warning outputs-should-not-contain-secrets: Outputs should not contain secrets. Found possible secret: output name 'db_password' suggests a secret [https://aka.ms/bicep/linter/outputs-should-not-contain-secrets]
/Users/pamelafox/Downloads/bicep 3/modules/postgresql/postgresql.bicep(71,29) : Warning outputs-should-not-contain-secrets: Outputs should not contain secrets. Found possible secret: secure parameter 'administratorPassword' [https://aka.ms/bicep/linter/outputs-should-not-contain-secrets]
/Users/pamelafox/Downloads/bicep 3/main.bicep(10,3) : Warning prefer-unquoted-property-names: Property names that are valid identifiers should be declared without quotation marks and accessed using dot notation. [https://aka.ms/bicep/linter/prefer-unquoted-property-names]
/Users/pamelafox/Downloads/bicep 3/main.bicep(11,3) : Warning prefer-unquoted-property-names: Property names that are valid identifiers should be declared without quotation marks and accessed using dot notation. [https://aka.ms/bicep/linter/prefer-unquoted-property-names]
/Users/pamelafox/Downloads/bicep 3/modules/app-service/app-service.bicep(79,18) : Warning simplify-interpolation: Remove unnecessary string interpolation. [https://aka.ms/bicep/linter/simplify-interpolation]

@cmaneu
Copy link
Member

cmaneu commented Mar 6, 2023

Thanks @pamelafox for this PR! I'll review it this week and add terraform support before merging.

  • You're right, we are not looking for completeness. We prefer doing 80% of the work right, and letting users update the templates themselves for the remaining 20% (we should simplify that path for them). So, I would not include memory-optimized tier. In the future, if we should introduce a tier, we should include Cosmos DB for Postgres
  • For now, I would remove the high availability from nubesgen but not from the template. So, if users want to use redundant, they only have one parameter to set in their main.bicep file to do it. In NubesGen v2, we plan to have an "options" system that would match offering these kinds of possibilities from the UI.
  • Yeah, I know. There are a few things security-wise we need to update. I also see in your output a few things that were accepted in earlier versions of Bicep and are now issuing a warning. I'll fix them as well. Thanks for the catch!

TODO

  • Review Bicep file & deployment
  • Remove high-availability option
  • Fix warnings related to quotes & interpolation
  • Implement Terraform version
  • Check UI
  • Publish

@pamelafox
Copy link
Member Author

pamelafox commented Mar 6, 2023

@cmaneu
Copy link
Member

cmaneu commented Mar 7, 2023

@pamelafox correct!

@cmaneu cmaneu self-requested a review March 8, 2023 09:09
@pamelafox
Copy link
Member Author

@cmaneu Okay, I made high ability into a param and updated the SLA accordingly.

@@ -943,7 +943,7 @@ <h3 class="text-lg leading-6 font-medium text-gray-900">
<div class="p-3 border rounded px-5 py-6 "
:class="components.database.size==='basic' ? 'bg-blue-900 text-white' : 'border-gray-300 bg-white text-black-500 hover:bg-blue-100'"
@click="components.database.size = 'basic'"
x-show="components.database.type!=='SQL_SERVER'"><span class="text-2xl">Basic</span>
x-show="components.database.type==='MYSQL'"><span class="text-2xl">Basic</span>
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? (I know this code is not perfect and hard to read..)

Copy link
Member Author

Choose a reason for hiding this comment

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

That change was because this Basic div is only for MySQL now, it used to be for both MYSQL and POSTGRES, but there were enough differences in the low tier (name, SLA) that I made a brand new div.

</div>
<div class="flex row-auto items-center mt-4"><i
class="fa fa-check-circle-o text-gray-400"></i>
<span class="ml-3 ">99,99% SLA with High Availability</span>
Copy link
Member

Choose a reason for hiding this comment

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

To update to reflect the new SLA.
Proposal: 99,9% SLA, High Availability available as an option

storage: {
storageSizeGB: 128
}
highAvailability: {
Copy link
Member

Choose a reason for hiding this comment

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

High availability doesn't work with burstable SKU (on top of the pricing we've already discussed). I suggest to

  1. Add a new parameter
@description('Enables High Availability deployment. Does not work on Burstable SKU')
param enableHighAvailability bool = false
  1. Replace that code with:
    highAvailability: {
      mode: (enableHighAvailability ? 'ZoneRedundant' : null)
    } 

Tested on my end.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I didn't realize it wasn't available for Burstable. Are you making that change or shall I? If you're going to default it to ZoneRedundant and not give the SameZone option, I think that description should say that. Do most people go for ZoneRedundant if they enable it, as a best practice?

@cmaneu cmaneu merged commit 0e611f1 into microsoft:main Mar 10, 2023
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.

None yet

2 participants