Feature/sjad #4473
Draft
Feature/sjad #4473
Conversation
This file contains 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
Support adding MySQL when run `azd init`
Fix typo by changing "DbMysql" to "DbMySql"
…or like this: "argetTypeNotSupported: Target resource type MICROSOFT.DBFORMYSQL/FLEXIBLESERVERS is not supported.".
2. Delete '1>&2' used for debug. 3. Add 'az tag create' to fix the problem about tag been deleted when creating service connector.
Use managed-identity instead of username and password
2. Update log about "failed to read spring application properties". 3. Fix bug about can not find frontend app and backend app at the same time. 4. Add service connector from aca to postgresql.
Use passwordless to connect to PostgreSQL in Azure Container Apps
…ble project (#103) * pre-check if it's a runnable project * when parent detection, add module check * fix UT * fix UT * fix UT * fix comments, and update the module check recursively * make pom artifact id is not same as the dir name * small fix * fix UT * small fix * clear mvn wrapper content * deprecate maven build hook, run mvn clean package before adding default dockerfile * remove unnecessary mvn wrapper files * remove unused func * fix UT and remove currentPomDir * introduce modulePoms * remove unused func, and refactor --------- Co-authored-by: haozhang <[email protected]>
|
@saragluna I noticed the This happens with both MongoDB and PostgreSQL. Is this a known issue? |
|
@JeffreyCA, thanks for the update, will fix it soon |
* fix the sjad azd add cannot work bug
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
|
@JeffreyCA the issue has been fixed in azure-javaee#104 |
Thank you, it seems to be working now. I tried to deploy a few of the samples from https://github.com/azure-javaee/azure-spring-boot-samples with the latest build but the two I tested both weren't working. They definitely used to work though.
In both cases it looks like some of the placeholder values aren't being substituted correctly? E.g. |
weikanglim
reviewed
Jan 13, 2025
| // 2. pom.Build.Plugins. | ||
| // 2.1. Only care about the groupId/artifactId/version. | ||
| // 2.2. Not include the default maven plugins (name with this patten: "maven-xxx-plugin"). | ||
| func createSimulatedEffectivePom(pomFilePath string) (pom, error) { |
There was a problem hiding this comment.
I'm of the opinion that createSimulatedEffectivePom is not a good idea in the long run. Should we keep this simple here, and just rely on mvn effective:pom, erroring out if maven isn't installed or errors out?
There is enough meat here to be a separate OSS library that effectively simulates POM. As a standalone project, or one that is close to such, the library needs to be documented how effective it is doing it, and how it differs in evaluation from mvn. The current implementation doesn't make it easy for anyone to see how this differs from a maven evaluation which is a source of unreliability.
I would imagine that we don't want downstream code to get the wrong information here because we ended up evaluating maven differently than normal.
There was a problem hiding this comment.
Here is the flow of using azd to deploy existing Spring Boot project:
- Step 1: Use
azd initto createazure.yamlby analyzing Spring Boot project. - Step 2: Check the content of
azure.yaml, updateazure.yamlif necessary. - Step 3: Use
azd upto deploy the project.
When mvn not exist in curtomer's Path, azd has 2 options:
- Exit
azd, and let customer intallmvn. - Try best to get the information of current project, even it's possible to have some mistake in some corner cases, then continue the
Step 2in above step.
As a Java developer, I prefer option 2.
@weikanglim , @saragluna , which one do you prefer?
weikanglim
reviewed
Jan 13, 2025
| "maven could not be found. Install either Maven or Maven Wrapper by " + | ||
| "visiting https://maven.apache.org/ or https://maven.apache.org/wrapper/", | ||
| ) | ||
| return getDownloadedMvnCommand(downloadedMavenVersion) |
There was a problem hiding this comment.
We want to avoid auto-acquiring build tools for users here. If a Java developer working on a maven project doesn't have maven installed, it isn't right for us to "fill the gap" by auto-acquiring maven for them.
This isn't something I'd expect an azure tool like azd to be doing; and I certainly don't think this is something our users would expect either.
There was a problem hiding this comment.
Hi, @weikanglim
Thanks for the feedback. But I feel it isn't a problem to download maven in azd:
- From customer's perspective: the azd downloaded maven won't affect customer's
Path. Customer's won't know the azd downloaded maven unless customer opened this folder:~/.azd/java. - From azd developer's perspective: download maven and use it to get effective pom, it's the implementation detail of azd. IMU, it's not rare that one tool download another tool.
- Current azd already has code to download other command line tools, for example: bicep command line tool. Here is the screenshot of related code:
There was a problem hiding this comment.
Hi @rujche, thanks for the reply.
From customer's perspective: the azd downloaded maven won't affect customer's Path
That is true, and I do recognize that. However, it's also possible that the maven downloaded could be incompatible with the maven project set up, resulting in a different expected result. Supposedly, if the maven version downloaded has a subtly difference in behavior due to version differences, this creates a very hard to troubleshoot scenario.
In the new change, we effectively also change the mvn that azd package uses to package a user's application. Imagine if the user had changed their PATH settings, that doesn't include mvn. User runs azd package, which subtly uses a different maven version to package their production application. This ended up breaking the application, but only at runtime, which creates this extremely difficult to troubleshoot issue.
From azd developer's perspective: download maven and use it to get effective pom, it's the implementation detail of azd.
In general, we want to limit our footprint of tools we download both from a security and maintenance perspective. Currently we have pack, gh and bicep as the tools we auto-acquire. There are good reasons behind each tool invocation, as we don't think the developers should need to install these tools. Here, we are discussing a scenario where a Java developer is working on a maven project without maven installed, which in my mind, is not a very valid scenario.
Sometimes doing the right thing for the customer is to not do more than we need to. Having Java developers install the tools that they want to use for the language they work on is perfectly fine in mind.
There was a problem hiding this comment.
Hi, @weikanglim
Sorry for my late response.
Supposedly, if the maven version downloaded has a subtly difference in behavior due to version differences, this creates a very hard to troubleshoot scenario.
In this case, customer should provide a maven wrapper. Wen maven wrapper is included in the repo, the downloaded maven will not be used. If customer didn't provide a maven wrapper, we (azd) will never get required information to deploy the app.
In the new change, we effectively also change the mvn that azd package uses to package a user's application.
If we consider this case, build from source will have this problem, too. build from source can not be used. we will never get the right docker image to deploy a maven project.
Having Java developers install the tools that they want to use for the language they work on is perfectly fine in mind.
Do you mean make related azd sub command failed when mvn is not installed in his environment?
JeffreyCA
reviewed
Jan 13, 2025
| @@ -704,7 +710,7 @@ module redisConn './modules/set-redis-conn.bicep' = { | |||
| } | |||
| {{- end}} | |||
|
|
|||
| {{- if .Services}} | |||
| {{- if (or .Services .DbCosmosMongo .DbRedis)}} | |||
There was a problem hiding this comment.
This actually ends up fixing #4574 and is an idea I had in mind as well. Any thoughts on this approach @weikanglim?
There was a problem hiding this comment.
@JeffreyCA Thanks for tagging me on this! At a first glance, I'd probably recommend not exporting the KeyVault secret if there are no services to bind to.
There was a problem hiding this comment.
That makes sense too. I created a separate PR to address this here: #4692
There was a problem hiding this comment.
I've merged the changes, so @saragluna you may need to revert this and pull the changes from main
weikanglim
reviewed
Jan 14, 2025
| @@ -20,6 +20,8 @@ type ServiceConfig struct { | |||
| ResourceName osutil.ExpandableString `yaml:"resourceName,omitempty"` | |||
| // The ARM api version to use for the service. If not specified, the latest version is used. | |||
| ApiVersion string `yaml:"apiVersion,omitempty"` | |||
| // The path to the parent directory of the project | |||
There was a problem hiding this comment.
Let's try to use the docker.context property to set the build context
weikanglim
reviewed
Jan 14, 2025
Comment on lines
+205
to
+219
| // if it's a java project without Dockerfile, we help to package jar and add a default Dockerfile for Docker build | ||
| if serviceConfig.Language == ServiceLanguageJava && serviceConfig.Docker.Path == "" { | ||
| mvnCli := maven.NewCli(exec.NewCommandRunner(nil)) | ||
| err := mvnCli.CleanPackage(ctx, serviceConfig.RelativePath, serviceConfig.Project.Path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defaultDockerfilePath, err := addDefaultDockerfileForJavaProject(serviceConfig.Name) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| serviceConfig.Docker = DockerProjectOptions{ | ||
| Path: defaultDockerfilePath, | ||
| } | ||
| } |
There was a problem hiding this comment.
I think you're running into microsoft/Oryx#2562 here.
In the meantime, I think we can consider testing out Paketo's Buildpack implementation. We can change the build on line 466 to point to paketobuildpacks/builder-jammy-base (only for Java) and see how well this works.
|
@JeffreyCA azure-javaee#106 fixes the issue. This will revert a previous PR. |
VSCode Extension Installation Instructions
|
Co-authored-by: haozhang <[email protected]>
* add uts for app_init, detect_confirm, infra_confirm, and scaffold
rujche
reviewed
Jan 20, 2025
| @@ -251,7 +307,7 @@ func (i *Initializer) InitFromApp( | |||
| var infraSpec *scaffold.InfraSpec | |||
| composeEnabled := i.features.IsEnabled(featureCompose) | |||
| if !composeEnabled { // backwards compatibility | |||
| spec, err := i.infraSpecFromDetect(ctx, detect) | |||
| spec, err := i.infraSpecFromDetect(ctx, &detect) | |||
There was a problem hiding this comment.
Hi, @weikanglim .
- Can we delete
spec.go, and useproject_config.goto generage bicep files directly? IMU, all information required to generated bicep files are contained inProjectConfiginproject_config.go - Can we delete
infraSpecFromDetect, and always useProjectConfigto generage bicep files? Delete the line in this diagram:
After deleted above one line and one node, the whole process will be much easier to understand:
JeffreyCA
reviewed
Jan 27, 2025
| @@ -67,4 +67,10 @@ output AZURE_RESOURCE_REDIS_ID string = resources.outputs.AZURE_RESOURCE_REDIS_I | |||
| {{- if .DbPostgres}} | |||
| output AZURE_RESOURCE_{{alphaSnakeUpper .DbPostgres.DatabaseName}}_ID string = resources.outputs.AZURE_RESOURCE_{{alphaSnakeUpper .DbPostgres.DatabaseName}}_ID | |||
| {{- end}} | |||
| {{- if .DbMySql}} | |||
There was a problem hiding this comment.
Should these start with AZURE_RESOURCE_, instead of AZURE_ like above?
| @@ -465,4 +773,13 @@ output AZURE_RESOURCE_REDIS_ID string = redis.outputs.resourceId | |||
| {{- if .DbPostgres}} | |||
| output AZURE_RESOURCE_{{alphaSnakeUpper .DbPostgres.DatabaseName}}_ID string = '${postgreServer.outputs.resourceId}/databases/{{.DbPostgres.DatabaseName}}' | |||
| {{- end}} | |||
| {{- if .DbMySql}} | |||
There was a problem hiding this comment.
Likewise, should these also start with AZURE_RESOURCE_? How are these outputs used?
|
Pull request contains merge conflicts. |
|
/azp run |
|
Pull request contains merge conflicts. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
To replace #4422. This PR will show the changes we made for java analyzer and bicep generation for azure resources
To make the review easier, break down into several separate PRs: