Try to use effective pom if possible #4710
Open
+434
−116
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
rujche
requested review from
ellismg,
wbreza,
vhvb1989,
hemarina,
weikanglim and
JeffreyCA
as code owners
There was a problem hiding this comment.
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- cli/azd/internal/appdetect/pom_test.go: Evaluated as low risk
Comments suppressed due to low confidence (1)
cli/azd/pkg/tools/maven/maven.go:241
- The new function 'EffectivePom' does not have test coverage. Please add tests to ensure its functionality.
func (cli *Cli) EffectivePom(ctx context.Context, pomPath string) (string, error) {
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
rujche
commented
Jan 20, 2025
cli/azd/internal/appdetect/pom.go
Outdated
| result, err = unmarshalPomFile(pomFilePath) | ||
| if err == nil { | ||
| result.path = filepath.Dir(pomFilePath) | ||
| // todo: handle pom, for example: <version>${project.version}<version> |
There was a problem hiding this comment.
Hi, @weikanglim
- My current plan is do this after current PR merged.
- Anyway, analyze the unmarshalled pom will be done. If you think
toSimulatedEffectivePomis not accurate, I can choose another func name, for example:pom := analyzeUnmarshalledPom(pom)
There was a problem hiding this comment.
Previous comment here: #4473 (comment)
I still strongly suggest we do not do this. A user is asking azd to scan a Java project that has pom.xml. The pom.xml clearly requires maven tooling installed to run. User also needs maven tooling installed to run azd package or azd up. The overall scenario requires the user to install maven.
I don't like the idea of trying to replicate maven evaluation because evaluation rules can be tricky to replicate; and I prefer tools that just work exactly, instead of tools that are subtly wrong. This can lead to downstream impacts that are harder to debug.
On the flip side, if we are confident in replicating maven evaluation rules, I would prefer removing the current effective-POM evaluation and have the native-go implementation take over -- it's cool work to have a 100% compatible maven evaluator in go, but I'm not confident that we can always replicate it since it's not a fully documented standard -- and it's one that changes over time.
There was a problem hiding this comment.
If we end up deciding removing the maven evaluation, perhaps pom_test.go can be removed along those lines as well.
There was a problem hiding this comment.
Hi, @weikanglim
I still strongly suggest we do not do this.
-
Thanks for the detailed explanation. Now I deleted the
todo. My current plan is usemvn help:effective-pomonly, will not analyze the pom file by go code. -
When
mvncommand not found, current behavior is to just unmarshall the pom file. Do you think it's necessary to exit analyze and ask customer to installmvn?
There was a problem hiding this comment.
If we end up deciding removing the maven evaluation, perhaps pom_test.go can be removed along those lines as well.
Agree. Now effective pom is generated by mvn help:effective-pom, it's not necessary to test it. I deleted pom_test.go.
There was a problem hiding this comment.
Do you think it's necessary to exit analyze and ask customer to install mvn?
Yes, I think it's better than doing a partial analysis
|
Hi, @saragluna , @haoozhang, @weikanglim BTW, I can't set @haoozhang as reviewer. @weikanglim , could you please help to set this? |
weikanglim
reviewed
Jan 22, 2025
weikanglim
reviewed
Jan 22, 2025
weikanglim
reviewed
Jan 22, 2025
cli/azd/pkg/tools/maven/maven.go
Outdated
Comment on lines
258
to
272
| inProject := false | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| if strings.HasPrefix(strings.TrimSpace(line), "<project") { | ||
| inProject = true | ||
| } else if strings.HasPrefix(strings.TrimSpace(line), "</project>") { | ||
| builder.WriteString(line) | ||
| break | ||
| } | ||
| if inProject { | ||
| if strings.TrimSpace(line) != "" { | ||
| builder.WriteString(line) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
a small suggestion here to avoid string allocations and streamline the code
Suggested change
| inProject := false | |
| for scanner.Scan() { | |
| line := scanner.Text() | |
| if strings.HasPrefix(strings.TrimSpace(line), "<project") { | |
| inProject = true | |
| } else if strings.HasPrefix(strings.TrimSpace(line), "</project>") { | |
| builder.WriteString(line) | |
| break | |
| } | |
| if inProject { | |
| if strings.TrimSpace(line) != "" { | |
| builder.WriteString(line) | |
| } | |
| } | |
| } | |
| inProject := false | |
| projectStart := regexp.MustCompile(`\s+<project`) | |
| projectEnd := regexp.MustCompile(`\s+</project>`) | |
| for scanner.Scan() { | |
| line := scanner.Text() | |
| if projectStart.MatchString(line) { | |
| inProject = true | |
| } else if projectEnd.MatchString(line) { | |
| builder.WriteString(line) | |
| break | |
| } | |
| if inProject { | |
| builder.WriteString(line) | |
| } | |
| } |
There was a problem hiding this comment.
FYI:
- I'll accept the changes by clicking
commit suggestion. - I'll fix the indent problem in line 271 in another commit.
- To test the changes, make sure it won't bring new bug, I'll recover
pom_test.go.
There was a problem hiding this comment.
FYI:
To test the changes, make sure it won't bring new bug, I'll recover pom_test.go.
Test failed.
Here is the screenshot when I debug:
- I'll fix the bug in another commit.
- It shows that
pom_test.gois useful, I'll keep it.
There was a problem hiding this comment.
I'll fix the bug in another commit.
Done in this commit: 90e806f
weikanglim
reviewed
Jan 22, 2025
Comment on lines
+12
to
+14
| type mavenProject struct { | ||
| pom pom | ||
| } |
There was a problem hiding this comment.
I don't see us adding more properties here... Are we okay keeping what we have to avoid code churn?
There was a problem hiding this comment.
I'm OK to delete mavenProject and use pom directly.
Hi, @saragluna , This option is required by you before. Now if you agree, I'll delete mavenProject and use pom directly.
weikanglim
reviewed
Jan 22, 2025
| @@ -0,0 +1,94 @@ | |||
| package appdetect | |||
There was a problem hiding this comment.
As a general go programming convention here, I would like to avoid adding more files like maven_project.go or
pom.go here. We can fit all under java.go. In the future, we could rename this to java_maven.go when gradle support is added.
There was a problem hiding this comment.
As a general go programming convention here
- Thanks for telling me that. I didn't know this convention before. BTW, could you please share related link about this convention? I guess there should be some official doc like this one: https://go.dev/doc/effective_go
- I use different files because:
2.1.javaandpomare different things in concept. Especially fortype pom struct, it's unmarshalled from pom.xml, it's a standalone concept.
2.2. Separate codes into multiple file can avoid one file too long. Too long file will make it hard to maintain.
vhvb1989
approved these changes
Jan 22, 2025
weikanglim
reviewed
Jan 23, 2025
Co-authored-by: Wei Lim <[email protected]>
Co-authored-by: Wei Lim <[email protected]>
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
Co-authored-by: Wei Lim <[email protected]>
This reverts commit 82726b3.
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.
Try to use effective pom if possible (both mvn and java command exists)