New feature: Update spring boot properties file for postgresql#4707
Closed
rujche wants to merge 21 commits intoAzure:mainfrom
rujche:feature/update-spring-boot-properties-file-for-postgresql
Closed
New feature: Update spring boot properties file for postgresql#4707rujche wants to merge 21 commits intoAzure:mainfrom rujche:feature/update-spring-boot-properties-file-for-postgresql
rujche wants to merge 21 commits intoAzure:mainfrom
rujche:feature/update-spring-boot-properties-file-for-postgresql
Conversation
rujche
requested review from
JeffreyCA,
ellismg,
hemarina,
vhvb1989,
wbreza and
weikanglim
as code owners
rujche
requested review from
Copilot,
saragluna and
weikanglim
and removed request for
JeffreyCA,
Copilot,
ellismg,
hemarina,
vhvb1989,
wbreza and
weikanglim
Member
Author
|
Hi, @saragluna , @weikanglim |
weikanglim
reviewed
Jan 17, 2025
| name: '{{bicepName .Name}}' | ||
| params: { | ||
| name: '{{.Name}}' | ||
| name: '{{containerAppName .Name}}' |
Contributor
There was a problem hiding this comment.
Curious about the change here -- we did intentionally move away from containerAppName .Name to .Name in #4434
Member
Author
There was a problem hiding this comment.
we did intentionally move away from containerAppName .Name to .Name in #4434
-
Do you mean change from
.NametocontainerAppName .Namein easy-init: allow for longer container app names #4434 ? -
In think the mistake (missing
containerAppName) is added in this PR: https://github.com/Azure/azure-dev/pull/4455/files
Contributor
There was a problem hiding this comment.
I see. I think when #4455 was made, we also started adding upfront name validation in azd add (UI layer, not at the schema layer).
Keeping it as .Name instead of containerAppName .Name (which does the truncation) transformation may help with #4653, so I think .Name still looks better directionally. What do you think?
Member
Author
There was a problem hiding this comment.
How about doing like this:
- In current PR, keep using
containerAppName .Namebecause it can solve the problem appeared in the sample project for current PR: https://github.com/azure-javaee/azure-spring-boot-samples/blob/788cc9a14f3fa2bfa826b0e875ba6dd729726dfa/postgresql/spring-cloud-azure-starter-jdbc-postgresql/spring-cloud-azure-postgresql-sample/pom.xml#L39 - For [Issue] Failed to deploy app when app name too long or different app name has same long prefix #4653, we can fix it in another PR. In that PR, the same name prefix should be considered,
Improve func: appendToCommaSeperatedValues
saragluna
reviewed
Jan 22, 2025
| } | ||
| type propertyMergeFunc func(string, string) string | ||
|
|
||
| const azureProfileName = "azure" |
Member
There was a problem hiding this comment.
what if in the user's application already have one azure profile?
Member
Author
There was a problem hiding this comment.
Good point.
In this case, original value of spring.profiles.active will not be changed. See these test cases:
And here is related source code:
Comment on lines
+21
to
+24
| const placeholderPostgresHost = "${POSTGRES_HOST}" | ||
| const placeholderPostgresPort = "${POSTGRES_PORT}" | ||
| const placeholderPostgresDatabase = "${POSTGRES_DATABASE}" | ||
| const placeholderPostgresUsername = "${POSTGRES_USERNAME}" |
Member
There was a problem hiding this comment.
These are the conventional env names that defined in azd.
Member
There was a problem hiding this comment.
I mean these should be defined in a common place, instead of being defined in java
…um of 125 characters. (lll)
// G101 : Potential hardcoded credentials (gosec)
Member
Author
|
Hi, @saragluna , @weikanglim |
…o to app_init.go.
…ng-boot-properties-file-for-postgresql
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
Member
Author
|
Hi, @saragluna , @weikanglim . |
weikanglim
reviewed
Feb 4, 2025
| return false | ||
| } | ||
|
|
||
| type RawDependency struct { |
Contributor
There was a problem hiding this comment.
If it's easier, we can just return the RawProject pom.xml representation and let the "app generation" part decide what to do with that information. I think we want to avoid having additional schema translations
weikanglim
reviewed
Feb 4, 2025
| } | ||
|
|
||
| func updatePropertyFile(filePath string, newProperties []property, function propertyMergeFunc) error { | ||
| err := createFileIfNotExist(filePath) |
Contributor
There was a problem hiding this comment.
Would it be simpler to just have a single function that opens the file, and just does a one-pass update?
I know that we've created a lot of functions here to attempt to break down the complexity. In doing so, they all "look right" individually, but if we zoom out to see the whole algorithm:
- If
createFileNotExistssucceeds, but we fail inreadProperties, we create an empty file - We also create the file again in
writeProperties
FWIW, this is related to what we had talked about creating too many functions.
weikanglim
reviewed
Feb 6, 2025
| break | ||
| } | ||
| } | ||
| if !isSpringBootProject { |
Contributor
There was a problem hiding this comment.
For non-spring boot projects, is there something needed for postgresql:postgresql?
weikanglim
reviewed
Feb 6, 2025
Comment on lines
+313
to
+314
| case "org.postgresql:postgresql", | ||
| "com.azure.spring:spring-cloud-azure-starter-jdbc-postgresql": |
Contributor
There was a problem hiding this comment.
Is it possible for both these dependencies to exist at the same time, and if so -- do we only want to generate once?
Member
Author
|
Closing this PR because now SJAD team plan to create azure.yaml by AI-based method. Similarity, application.properties will be updated by AI-based method, too. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1. Description
New feature: Update spring boot properties file for postgresql.
2. Try this feature
Link to sample project: https://github.com/azure-javaee/azure-spring-boot-samples/blob/788cc9a14f3fa2bfa826b0e875ba6dd729726dfa/postgresql/spring-cloud-azure-starter-jdbc-postgresql/spring-cloud-azure-postgresql-sample/pom.xml#L39
Screenshot before running
azd init:azd init:todos in current PR, my current plan is do them after current PR merged.