search

Azure / azure-dev

A developer CLI that reduces the time it takes for you to get started on Azure. The Azure Developer CLI (azd) provides a set of developer-friendly commands that map to key stages in your workflow - code, build, deploy, monitor, repeat.
https://aka.ms/azd
MIT License
373 stars 168 forks source link

refactor todo-java template and fix bugs. #1122

Closed wangmingliang-ms closed 1 year ago

wangmingliang-ms commented 1 year ago

This PR refactored code and fixed some issues (including those in comments of PR https://github.com/Azure/azure-dev/pull/799 and several others) of SimpleTodo templates for Java AppService and Container Apps.

jongio commented 1 year ago

@v-xuto - Can you please run a test pass on all java templates? Thanks!

I will generate the templates now...

jongio commented 1 year ago

/azp run azure-dev - repoman

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).
weikanglim commented 1 year ago

Started running template tests validation: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1980514&view=results

FYI @jongio

weikanglim commented 1 year ago

@jdubois Could use you or your team's insights on what might be idiomatic Java or otherwise based on your experiences. Otherwise, code changes looks fine pending validation.

wangmingliang-ms commented 1 year ago

@brunoborges you are right, Lombok is removed.

brunoborges commented 1 year ago

@brunoborges you are right, Lombok is removed.

Awesome, thank you.

What is the Java version set for the source code and the version of the JDK used in the images? Is it 17?

wangmingliang-ms commented 1 year ago

What is the Java version set for the source code and the version of the JDK used in the images? Is it 17?

yes, it's 17 (in both pom.xml and api-appservice-java.bicep). I didn't modify it.

v-xuto commented 1 year ago

@v-xuto - Can you please run a test pass on all java templates? Thanks!

I will generate the templates now...

@jongio We have tested the java templates (todo-java-mongo and todo-java-mongo-aca) only in Windows Desktop, both of them test results are PASS.

brunoborges commented 1 year ago

Can you make sure they also work fine on Linux? You may use WSL

v-xuto commented 1 year ago

Can you make sure they also work fine on Linux? You may use WSL

We have tested the java templates in WSL, the results are PASS.

weikanglim commented 1 year ago

/azp run azure-dev - repoman

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).
weikanglim commented 1 year ago

Re-running repoman after latest repoman CI changes

weikanglim commented 1 year ago

/azp run azure-dev - repoman

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).
azure-sdk commented 1 year ago

Repoman Generation Results

Repoman pushed changes to remotes for the following projects:

Project: todo-csharp-cosmos-sql

Remote: azure-samples-staging

Branch: pr/1122

You can initialize this project with:

azd init -t Azure-Samples/todo-csharp-cosmos-sql -b pr/1122

View Changes | Compare Changes

Project: todo-csharp-sql-swa-func

Remote: azure-samples-staging

Branch: pr/1122

You can initialize this project with:

azd init -t Azure-Samples/todo-csharp-sql-swa-func -b pr/1122

View Changes | Compare Changes

Project: todo-csharp-sql

Remote: azure-samples-staging

Branch: pr/1122

You can initialize this project with:

azd init -t Azure-Samples/todo-csharp-sql -b pr/1122

View Changes | Compare Changes

Project: todo-java-mongo-aca

Remote: azure-samples-staging

Branch: pr/1122

You can initialize this project with:

azd init -t Azure-Samples/todo-java-mongo-aca -b pr/1122

View Changes | Compare Changes

Project: todo-java-mongo

Remote: azure-samples-staging

Branch: pr/1122

You can initialize this project with:

azd init -t Azure-Samples/todo-java-mongo -b pr/1122

View Changes | Compare Changes

Project: todo-nodejs-mongo-aca

Remote: azure-samples-staging

Branch: pr/1122

You can initialize this project with:

azd init -t Azure-Samples/todo-nodejs-mongo-aca -b pr/1122

View Changes | Compare Changes

Project: todo-nodejs-mongo-swa-func

Remote: azure-samples-staging

Branch: pr/1122

You can initialize this project with:

azd init -t Azure-Samples/todo-nodejs-mongo-swa-func -b pr/1122

View Changes | Compare Changes

Project: todo-nodejs-mongo

Remote: azure-samples-staging

Branch: pr/1122

You can initialize this project with:

azd init -t Azure-Samples/todo-nodejs-mongo -b pr/1122

View Changes | Compare Changes

Project: todo-nodejs-mongo-terraform

Remote: azure-samples-staging

Branch: pr/1122

You can initialize this project with:

azd init -t Azure-Samples/todo-nodejs-mongo-terraform -b pr/1122

View Changes | Compare Changes

Project: todo-python-mongo-aca

Remote: azure-samples-staging

Branch: pr/1122

You can initialize this project with:

azd init -t Azure-Samples/todo-python-mongo-aca -b pr/1122

View Changes | Compare Changes

Project: todo-python-mongo-swa-func

Remote: azure-samples-staging

Branch: pr/1122

You can initialize this project with:

azd init -t Azure-Samples/todo-python-mongo-swa-func -b pr/1122

View Changes | Compare Changes

Project: todo-python-mongo

Remote: azure-samples-staging

Branch: pr/1122

You can initialize this project with:

azd init -t Azure-Samples/todo-python-mongo -b pr/1122

View Changes | Compare Changes

Project: todo-python-mongo-terraform

Remote: azure-samples-staging

Branch: pr/1122

You can initialize this project with:

azd init -t Azure-Samples/todo-python-mongo-terraform -b pr/1122

View Changes | Compare Changes

wangmingliang-ms commented 1 year ago

@weikanglim @jongio is there anything I should fix?

weikanglim commented 1 year ago

@wangmingliang-ms No, the test failures are unrelated to your changes. We were in the middle of a release. Overall, the changes look good to me, and I'll merge it at the next appropriate window.

wangmingliang-ms commented 1 year ago

@wangmingliang-ms Can you verify what additional changes we might need so that running ./mvnw -P openapigen compile doesn't result in a huge diff in the java files?

The huge diff you mentioned should be mainly caused by 2 reasons:

  1. Did not use useTags=true when generating code, resulting in only one ListsApi generated, instead of the separate ItemsApi and ListsApi we expected, I submitted another commit to fix this problem see https://github.com/Azure/azure-dev/pull/1122/commits/7647439d111f5aa960ffbac5350bc749f87befa0.
  2. Code style: the generated code uses 2 spaces indention instead of the 4 spaces commonly used in the Java world. In addition, the generated code also contains some completely unused imports, see below screenshot. I didn't find proper settings to fix these 2 problems of OpenApi generator right part is the code generated by OpenApi generator. image
wangmingliang-ms commented 1 year ago

The code style issue can be solved by some maven plugins e.g. io.spring.javaformat:spring-javaformat-maven-plugin. see commit https://github.com/Azure/azure-dev/pull/1122/commits/ce21ce43d988f22c73c12e8569e17e1afcab01f3
but none of them can organize the imports.

jdubois commented 1 year ago

For formatting, please have a look at https://github.com/jhipster/prettier-java (it’s under JHipster, to be transparent). You’ll have perfect formatting and it works with IDEs and CI easily as it’s Prettier.

weikanglim commented 1 year ago

@wangmingliang-ms The formatting from javaformat-maven ended up not being very readable:

https://github.com/Azure/azure-dev/blob/c6fdca1817f3cc4e48a46465445bc86b6eca1cae/templates/todo/api/java/src/main/java/com/microsoft/azure/simpletodo/api/ItemsApi.java#L45-L60

So I opted for using prettier which has a much better result:

https://github.com/Azure/azure-dev/blob/9966e027bf8be8e43f9bee2b1cde7cde18e76558/templates/todo/api/java/src/main/java/com/microsoft/azure/simpletodo/api/ItemsApi.java#L50-L65

After these changes, I can see the minimal codegen diff of only the intended, modified changes to our model classes, so I think we're in a good spot.

weikanglim commented 1 year ago

@jdubois @wangmingliang-ms One final thing to check about prettier, the import order seems to not prefer import sections, and instead always alphabetical:

https://github.com/Azure/azure-dev/blob/9966e027bf8be8e43f9bee2b1cde7cde18e76558/templates/todo/api/java/src/main/java/com/microsoft/azure/simpletodo/api/ItemsApi.java#L8-L30

Which seems like it's a hardcoded and opinionated import format according to their readme. I don't have a concern here as long as we have a consistent format. But do you feel strongly otherwise?

I just wanted to get to a point where the user using the sample could understand the openapigen diff easily, so if they wanted to use the template as a starting point for their application, it would make total sense. I think that's accomplished here but let me know otherwise. Would love to get this merged sooner than later.

wangmingliang-ms commented 1 year ago

I think it's OK, no style can satisfy all users, sections or not does not affect readability actually. What's more, google code style is almost the most popular.