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

Add azd #58

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Add azd #58

wants to merge 17 commits into from

Conversation

SandraAhlgrimm
Copy link
Collaborator

Create the infrastructure as requested in #53

@SandraAhlgrimm SandraAhlgrimm linked an issue Jun 11, 2024 that may be closed by this pull request
6 tasks
Copy link
Collaborator

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

What are all these *.bicep files and were do they come from?
(I imagine they come from here: https://learn.microsoft.com/en-us/azure/azure-resource-manager/bicep/overview?tabs=bicep)

They look pretty cryptic and extensive and if they are need to deploy on Azure we should explain what do they do and some reference to build your own.

infra/main.bicep Outdated Show resolved Hide resolved
var _quarkusContainerAppName = !empty(quarkusContainerAppName) ? quarkusContainerAppName : take('${abbreviations.appContainerApps}quarkus-${environmentName}', 32)
var _springBootContainerAppName = !empty(springBootContainerAppName) ? springBootContainerAppName : take('${abbreviations.appContainerApps}spring-boot-${environmentName}', 32)
var _postgresFlexibleServerName = !empty(postgresFlexibleServerName) ? postgresFlexibleServerName : take(toLower('${abbreviations.dBforPostgreSQLServers}${take(environmentName, 44)}-${resourceToken}'), 63)
var _keyVaultName = !empty(keyVaultName) ? keyVaultName : take('${abbreviations.keyVaultVaults}${take(alphaNumericEnvironmentName, 8)}${resourceToken}', 24)

Choose a reason for hiding this comment

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

I think it is a better practice to provide these names only in the one line where you need them. If you store them as variables, you may be tempted to reference the variable name instead of module.outputs.name, and then Bicep won't realize there's a dependency between two modules.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand your concern. The generation of names and counting of how many characters are truncated is quite complex and prone to mistakes. I prefer to have all name wrangling in the same place for the main file. Also, the extra long lines make for less readable resources & modules.
Misuse of those variables is mitigated by their '_' prefix.

var _keyVaultSecrets = [
{
name: 'postgres-admin-password'
value: postgresAdminPassword

Choose a reason for hiding this comment

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

We are trying to move to passwordless auth for PG but that is a larger change (as it also effects code)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it wouldn't be a great fit for this example as it is already very complex. Or WDYT @brunobat and @jeanbisutti ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would propose we make this a separate issue. I'm not sure the exact impact it would have on the native part,

infra/main.bicep Outdated Show resolved Hide resolved
}

/* Give access to the keyvault to the current identity */
module principalKeyVaultAccess './core/security/keyvault-access.bicep' = {

Choose a reason for hiding this comment

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

See my comment in chat re moving to RBAC

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dbroeglin
Copy link
Collaborator

What are all these *.bicep files and were do they come from? (I imagine they come from here: https://learn.microsoft.com/en-us/azure/azure-resource-manager/bicep/overview?tabs=bicep)

They look pretty cryptic and extensive and if they are need to deploy on Azure we should explain what do they do and some reference to build your own.

They all come from https://github.com/Azure-Samples/azd-starter-bicep I did an initial cleanup and left some that we might use. My latest commit should have cleaned any unused bicep files. This directory structure is pretty common in AZD templates out there and naming is pretty self-explanatory.

@SandraAhlgrimm
Copy link
Collaborator Author

this needs to be updated to the latest version

@SandraAhlgrimm
Copy link
Collaborator Author

add documentation links to the readme and explain high level

@SandraAhlgrimm SandraAhlgrimm marked this pull request as draft July 8, 2024 09:43
@SandraAhlgrimm SandraAhlgrimm marked this pull request as ready for review September 19, 2024 12:24
@brunobat
Copy link
Collaborator

@SandraAhlgrimm can you please explain in the docs or even the readme the structure of the infra repo and what are these .bicep files?... Maybe with a link to their documentation?

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.

Infrastructure as code with azd
4 participants