-
Notifications
You must be signed in to change notification settings - Fork 268
Add Java ACA and AppService templates #799
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
Conversation
wbreza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall. I would recommend someone with more Java experience review the Java specific bits.
|
@jdubois, @brunoborges, @roryp or @bbenz if any one of you can help sanity check the Java portion of changes to make sure nothing is amiss, that'd be appreciated. I'm still waiting to get a |
|
@weikanglim did you copy/paste the Java application I coded about 6 months ago? If that's the case, it should be all good. Maybe we can update it to the latest Spring Boot 2.x release, that shouldn't change anything (and remove a few CVE advisories). |
|
@jdubois That is correct. All the Java code was lifted from your existing implementation. Only slight modifications based on app properties, keyvault and appinsights integration. Upgraded spring-starter dependency as you suggested. |
|
@weikanglim then that should be all good. I'll have a look to update it to the latest Spring Boot version, but that's not urgent, it's already very recent. |
Repoman Generation ResultsRepoman pushed changes to remotes for the following projects: Project: todo-csharp-cosmos-sqlRemote: azure-samples-stagingBranch: pr/799You can initialize this project with: azd init -t Azure-Samples/todo-csharp-cosmos-sql -b pr/799View Changes | Compare Changes Project: todo-csharp-sqlRemote: azure-samples-stagingBranch: pr/799You can initialize this project with: azd init -t Azure-Samples/todo-csharp-sql -b pr/799View Changes | Compare Changes Project: todo-java-mongo-acaRemote: azure-samples-stagingBranch: pr/799You can initialize this project with: azd init -t Azure-Samples/todo-java-mongo-aca -b pr/799View Changes | Compare Changes Project: todo-java-mongoRemote: azure-samples-stagingBranch: pr/799You can initialize this project with: azd init -t Azure-Samples/todo-java-mongo -b pr/799View Changes | Compare Changes Project: todo-nodejs-mongo-acaRemote: azure-samples-stagingBranch: pr/799You can initialize this project with: azd init -t Azure-Samples/todo-nodejs-mongo-aca -b pr/799View Changes | Compare Changes Project: todo-nodejs-mongo-swa-funcRemote: azure-samples-stagingBranch: pr/799You can initialize this project with: azd init -t Azure-Samples/todo-nodejs-mongo-swa-func -b pr/799View Changes | Compare Changes Project: todo-nodejs-mongoRemote: azure-samples-stagingBranch: pr/799You can initialize this project with: azd init -t Azure-Samples/todo-nodejs-mongo -b pr/799View Changes | Compare Changes Project: todo-nodejs-mongo-terraformRemote: azure-samples-stagingBranch: pr/799You can initialize this project with: azd init -t Azure-Samples/todo-nodejs-mongo-terraform -b pr/799View Changes | Compare Changes Project: todo-python-mongo-acaRemote: azure-samples-stagingBranch: pr/799You can initialize this project with: azd init -t Azure-Samples/todo-python-mongo-aca -b pr/799View Changes | Compare Changes Project: todo-python-mongo-swa-funcRemote: azure-samples-stagingBranch: pr/799You can initialize this project with: azd init -t Azure-Samples/todo-python-mongo-swa-func -b pr/799View Changes | Compare Changes Project: todo-python-mongoRemote: azure-samples-stagingBranch: pr/799You can initialize this project with: azd init -t Azure-Samples/todo-python-mongo -b pr/799View Changes | Compare Changes Project: todo-python-mongo-terraformRemote: azure-samples-stagingBranch: pr/799You can initialize this project with: azd init -t Azure-Samples/todo-python-mongo-terraform -b pr/799View Changes | Compare Changes |
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsStandalone Binary
Container |
|
I will test today and let you know
regards
Rory
…On Wed, Oct 5, 2022 at 12:30 AM Wei Lim ***@***.***> wrote:
@jdubois <https://github.com/jdubois>, @brunoborges
<https://github.com/brunoborges>, @roryp <https://github.com/roryp> or
@bbenz <https://github.com/bbenz> if any one of you can help sanity check
the Java portion of changes to make sure nothing is amiss, that'd be
appreciated.
I'm still waiting to get a daily azd build published so that you can run
without having to compile a dev build locally. In the meantime, it requires
following contributing.md
<https://github.com/Azure/azure-dev/blob/main/cli/azd/CONTRIBUTING.md> to
build an azd locally..
—
Reply to this email directly, view it on GitHub
<#799 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABQXZAA242SDPPKSNAJ3DRTWBSV2FANCNFSM6AAAAAAQ33WMSU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
It seems the docker device runs out of space for the pr/551
I suggest creating the docker mount with double the storage space.
#4 extracting
sha256:466cf5840e59205c075ee1121d8cfb41cac9c8d64d1fade97d9fa35f34460169 3.9s
#4 ERROR: failed to register layer: Error processing tar file(exit status
1): write /usr/lib/jvm/msopenjdk-17/lib/modules: no space left on device
------
[build 1/9] FROM
***@***.***:be60f04045428da1c8604897aaf96d7a178e4f44d07cc2981e6c49dc0a3b24b5
:
------
failed to register layer: Error processing tar file(exit status 1): write
/usr/lib/jvm/msopenjdk-17/lib/modules: no space left on device
: exit status 1
Rorys-MacBook-Pro-3:todo-java-mongo-aca rorypreddy$
…On Wed, Oct 5, 2022 at 12:30 AM Wei Lim ***@***.***> wrote:
@jdubois <https://github.com/jdubois>, @brunoborges
<https://github.com/brunoborges>, @roryp <https://github.com/roryp> or
@bbenz <https://github.com/bbenz> if any one of you can help sanity check
the Java portion of changes to make sure nothing is amiss, that'd be
appreciated.
I'm still waiting to get a daily azd build published so that you can run
without having to compile a dev build locally. In the meantime, it requires
following contributing.md
<https://github.com/Azure/azure-dev/blob/main/cli/azd/CONTRIBUTING.md> to
build an azd locally..
—
Reply to this email directly, view it on GitHub
<#799 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABQXZAA242SDPPKSNAJ3DRTWBSV2FANCNFSM6AAAAAAQ33WMSU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Full failure log for pr/551 for container apps template:
Rorys-MacBook-Pro-3:todo-java-mongo-aca rorypreddy$ azd up
? Please enter a new environment name: todocosmosrpza
? Please select an Azure location to use: 41. (US) East US 2 (eastus2)
? Please select an Azure Subscription to use: 23. ca-ropreddy-demo-test
(296f38ef-7319-4a2b-ab48-2f38cb21966e)
Infrastructure provisioning plan completed successfully
Provisioning Azure resources can take some time.
You can view detailed progress in the Azure Portal:
https://portal.azure.com/#blade/HubsExtension/DeploymentDetailsBlade/overview/id/%2Fsubscriptions%2F296f38ef-7319-4a2b-ab48-2f38cb21966e%2Fproviders%2FMicrosoft.Resources%2Fdeployments%2Ftodocosmosrpza
Created Resource group: rg-todocosmosrpza
Created Key vault: kv-ozejguycfkida
Created Log Analytics workspace: log-ozejguycfkida
Created Application Insights: appi-ozejguycfkida
Created Portal dashboard: dash-ozejguycfkida
Created Container Apps Environment: cae-ozejguycfkida
Created Container App: ca-api-ozejguycfkida
Created Azure Cosmos DB: cosmos-ozejguycfkida
Created Container App: ca-web-ozejguycfkida
Azure resource provisioning completed successfully
Deploying service api...
Error: deploying service: packaging service api: building container: api at
.: building image: exit code: 1, stdout: , stderr: #1 [internal] load build
definition from Dockerfile
#1 sha256:fea6948161f7e274462dc7e31ba847b9d8313b1bb5da0f8bfff5969805da2c4c
#1 transferring dockerfile: 777B done
#1 DONE 0.0s
#2 [internal] load .dockerignore
#2 sha256:86779696bd4f4c763a30f89cd74f33ce2a9dce8c7318b574cc918e1807f0460d
#2 transferring context: 2B done
#2 DONE 0.0s
#3 [internal] load metadata for mcr.microsoft.com/openjdk/jdk:17-mariner
#3 sha256:12da1caa7cb1e091da203d54e325c61d9dc8868901554bda381e714294745d9a
#3 DONE 3.7s
#6 [internal] load build context
#6 sha256:7e1d23fea746ca32373c4dd81c12ef1f6db700441d7bd37e8a50243d2ec39958
#6 transferring context: 132.34kB 0.0s done
#6 DONE 0.0s
#4 [build 1/9] FROM
***@***.***:be60f04045428da1c8604897aaf96d7a178e4f44d07cc2981e6c49dc0a3b24b5
#4 sha256:8c1a155b388143167348573bd9e32802c92d6857d1230f43b95dc2583eb670b9
#4 resolve
***@***.***:be60f04045428da1c8604897aaf96d7a178e4f44d07cc2981e6c49dc0a3b24b5
done
#4 sha256:466cf5840e59205c075ee1121d8cfb41cac9c8d64d1fade97d9fa35f34460169
149.69MB / 149.69MB 112.3s done
#4 sha256:be60f04045428da1c8604897aaf96d7a178e4f44d07cc2981e6c49dc0a3b24b5
742B / 742B done
#4 sha256:1e4ab1988ec292ad33262cfc7922458c62c8ecc5f0b43e8a518bdad9688f9c7c
5.88kB / 5.88kB done
#4 sha256:7f0517a4d0b40accc745957dd7518369b86e064df31d5c75fd94af60b0dd5e4e
28.09MB / 28.09MB 15.2s done
#4 extracting
sha256:7f0517a4d0b40accc745957dd7518369b86e064df31d5c75fd94af60b0dd5e4e
1.1s done
#4 extracting
sha256:466cf5840e59205c075ee1121d8cfb41cac9c8d64d1fade97d9fa35f34460169 3.9s
#4 ERROR: failed to register layer: Error processing tar file(exit status
1): write /usr/lib/jvm/msopenjdk-17/lib/modules: no space left on device
------
[build 1/9] FROM
***@***.***:be60f04045428da1c8604897aaf96d7a178e4f44d07cc2981e6c49dc0a3b24b5
:
------
failed to register layer: Error processing tar file(exit status 1): write
/usr/lib/jvm/msopenjdk-17/lib/modules: no space left on device
: exit code: 1, stdout: , stderr: #1 [internal] load build definition from
Dockerfile
#1 sha256:fea6948161f7e274462dc7e31ba847b9d8313b1bb5da0f8bfff5969805da2c4c
#1 transferring dockerfile: 777B done
#1 DONE 0.0s
#2 [internal] load .dockerignore
#2 sha256:86779696bd4f4c763a30f89cd74f33ce2a9dce8c7318b574cc918e1807f0460d
#2 transferring context: 2B done
#2 DONE 0.0s
#3 [internal] load metadata for mcr.microsoft.com/openjdk/jdk:17-mariner
#3 sha256:12da1caa7cb1e091da203d54e325c61d9dc8868901554bda381e714294745d9a
#3 DONE 3.7s
#6 [internal] load build context
#6 sha256:7e1d23fea746ca32373c4dd81c12ef1f6db700441d7bd37e8a50243d2ec39958
#6 transferring context: 132.34kB 0.0s done
#6 DONE 0.0s
#4 [build 1/9] FROM
***@***.***:be60f04045428da1c8604897aaf96d7a178e4f44d07cc2981e6c49dc0a3b24b5
#4 sha256:8c1a155b388143167348573bd9e32802c92d6857d1230f43b95dc2583eb670b9
#4 resolve
***@***.***:be60f04045428da1c8604897aaf96d7a178e4f44d07cc2981e6c49dc0a3b24b5
done
#4 sha256:466cf5840e59205c075ee1121d8cfb41cac9c8d64d1fade97d9fa35f34460169
149.69MB / 149.69MB 112.3s done
#4 sha256:be60f04045428da1c8604897aaf96d7a178e4f44d07cc2981e6c49dc0a3b24b5
742B / 742B done
#4 sha256:1e4ab1988ec292ad33262cfc7922458c62c8ecc5f0b43e8a518bdad9688f9c7c
5.88kB / 5.88kB done
#4 sha256:7f0517a4d0b40accc745957dd7518369b86e064df31d5c75fd94af60b0dd5e4e
28.09MB / 28.09MB 15.2s done
#4 extracting
sha256:7f0517a4d0b40accc745957dd7518369b86e064df31d5c75fd94af60b0dd5e4e
1.1s done
#4 extracting
sha256:466cf5840e59205c075ee1121d8cfb41cac9c8d64d1fade97d9fa35f34460169 3.9s
#4 ERROR: failed to register layer: Error processing tar file(exit status
1): write /usr/lib/jvm/msopenjdk-17/lib/modules: no space left on device
------
[build 1/9] FROM
***@***.***:be60f04045428da1c8604897aaf96d7a178e4f44d07cc2981e6c49dc0a3b24b5
:
------
failed to register layer: Error processing tar file(exit status 1): write
/usr/lib/jvm/msopenjdk-17/lib/modules: no space left on device
: exit status 1
…On Wed, Oct 5, 2022 at 12:30 AM Wei Lim ***@***.***> wrote:
@jdubois <https://github.com/jdubois>, @brunoborges
<https://github.com/brunoborges>, @roryp <https://github.com/roryp> or
@bbenz <https://github.com/bbenz> if any one of you can help sanity check
the Java portion of changes to make sure nothing is amiss, that'd be
appreciated.
I'm still waiting to get a daily azd build published so that you can run
without having to compile a dev build locally. In the meantime, it requires
following contributing.md
<https://github.com/Azure/azure-dev/blob/main/cli/azd/CONTRIBUTING.md> to
build an azd locally..
—
Reply to this email directly, view it on GitHub
<#799 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABQXZAA242SDPPKSNAJ3DRTWBSV2FANCNFSM6AAAAAAQ33WMSU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Created Container Apps Environment: cae-ozejguycfkida
Error: deploying infrastructure: error deploying infrastructure: failed
deploying: failed running az deployment sub create:
Deployment Error Details:
ConflictError: A vault with the same name already exists in deleted state.
You need to either recover or purge existing key vault. Follow this link
https://go.microsoft.com/fwlink/?linkid=2149745 for more information on
soft delete.next issue: I tried to manually delete the resource group after
the failure and cant recreate the app as the keyvault needs to be manually
deleted:
…On Wed, Oct 5, 2022 at 12:30 AM Wei Lim ***@***.***> wrote:
@jdubois <https://github.com/jdubois>, @brunoborges
<https://github.com/brunoborges>, @roryp <https://github.com/roryp> or
@bbenz <https://github.com/bbenz> if any one of you can help sanity check
the Java portion of changes to make sure nothing is amiss, that'd be
appreciated.
I'm still waiting to get a daily azd build published so that you can run
without having to compile a dev build locally. In the meantime, it requires
following contributing.md
<https://github.com/Azure/azure-dev/blob/main/cli/azd/CONTRIBUTING.md> to
build an azd locally..
—
Reply to this email directly, view it on GitHub
<#799 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABQXZAA242SDPPKSNAJ3DRTWBSV2FANCNFSM6AAAAAAQ33WMSU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
after fixeding the 2 errors (adding more docker storage locally on my
laptop, and manually purging the keyvault) I have confirmed the app is now
working perfectly [image: image.png]
…On Wed, Oct 5, 2022 at 12:30 AM Wei Lim ***@***.***> wrote:
@jdubois <https://github.com/jdubois>, @brunoborges
<https://github.com/brunoborges>, @roryp <https://github.com/roryp> or
@bbenz <https://github.com/bbenz> if any one of you can help sanity check
the Java portion of changes to make sure nothing is amiss, that'd be
appreciated.
I'm still waiting to get a daily azd build published so that you can run
without having to compile a dev build locally. In the meantime, it requires
following contributing.md
<https://github.com/Azure/azure-dev/blob/main/cli/azd/CONTRIBUTING.md> to
build an azd locally..
—
Reply to this email directly, view it on GitHub
<#799 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABQXZAA242SDPPKSNAJ3DRTWBSV2FANCNFSM6AAAAAAQ33WMSU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I just tested the github actions via a private repo and it works great! When will you publish the live template @weikanglim ? |
Add SimpleTodo templates for Java AppService and Container Apps Co-authored-by: @jdubois and @brunoborges
| Optional<TodoItem> todoItem = getTodoItem(listId, itemId); | ||
| if (todoItem.isPresent()) { | ||
| todoItemRepository.deleteById(itemId); | ||
| return ResponseEntity.status(HttpStatus.NO_CONTENT).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResponseEntity.noContent().build();
| Optional<TodoItem> todoItem = getTodoItem(listId, itemId); | ||
| if (todoItem.isPresent()) { | ||
| todoItemRepository.deleteById(itemId); | ||
| return ResponseEntity.status(HttpStatus.NO_CONTENT).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResponseEntity.noContent().build()
| this.name = name; | ||
| } | ||
|
|
||
| public TodoItem description(String description) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't usually use both fluent/chainable accessors and getter/setters at the same time.
We suggest to keep only getters/setters because they are more generally used and supported by other tools such as jackson
| Objects.equals(this.description, todoItem.description) && | ||
| Objects.equals(this.state, todoItem.state) && | ||
| Objects.equals(this.dueDate, todoItem.dueDate) && | ||
| Objects.equals(this.completedDate, todoItem.completedDate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the same Todo item, no matter it is in the initial state, in progress, or done.
that is, I don't think properties like state should be used to implement equals()/hashCode() methods, because state is naturally to be updated for a todo item.
I would suggest keep only listId and id.
| TodoList todoList = (TodoList) o; | ||
| return Objects.equals(this.id, todoList.id) && | ||
| Objects.equals(this.name, todoList.name) && | ||
| Objects.equals(this.description, todoList.description); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here,
| @Override | ||
| public ResponseEntity<List<TodoItem>> getItemsByListId(String listId, BigDecimal top, BigDecimal skip) { | ||
| if (top == null) { | ||
| top = new BigDecimal(20); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use @RequestParam(defaultValue=20) to simplify code here and other.
https://www.baeldung.com/spring-request-param#a-default-value-for-the-request-parameter
| public interface TodoItemRepository extends MongoRepository<TodoItem, String> { | ||
|
|
||
| @Query("{ 'listId' : ?0 }") | ||
| List<TodoItem> findTodoItemsByTodoList(String listId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you don't need to add this @Query if this method is renamed as
findTodoItemsBy**ListId**(String listId);, because TodoItem itself has a property named listId, spring-data would generate the query by itself.
| public ResponseEntity<TodoList> updateListById(String listId, TodoList todoList) { | ||
| return todoListRepository | ||
| .findById(listId) | ||
| .map(t -> ResponseEntity.ok(todoListRepository.save(t))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be
ResponseEntity.ok(todoListRepository.save(**todoList**)))
otherwise the todolist won't be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also make sure the listId is set into todoList before saving. a new Todo List would be created after saving if its id is not set.
| return todoListRepository | ||
| .findById(listId) | ||
| .map(t -> ResponseEntity.ok(todoListRepository.save(t))) | ||
| .orElseGet(() -> ResponseEntity.badRequest().build()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure, but I think it should return notFound() here.
| @JsonCreator | ||
| public static TodoState fromValue(String value) { | ||
| for (TodoState b : TodoState.values()) { | ||
| if (b.value.equals(value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend to use equalsIgnoreCase() to be more robust.
| } | ||
|
|
||
| @Override | ||
| public ResponseEntity<Void> updateItemsStateByListId(String listId, TodoState state, List<String> requestBody) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove param requestBody if not used.
|
@wangmingliang-ms I agree with your PR review but be careful: many of the issues come because we inherit com.microsoft.azure.simpletodo.api.ListsApi which clearly isn't perfect, but is automatically generated using https://openapi-generator.tech (and to be more specific: their Spring code generator). As a result, unfortunately some of the methods cannot be changed, as they are inherited from this base class: sometimes that's because our Swagger contract isn't good (I found 3 errors in the contract that way), sometimes it's because OpenAPI doesn't generate perfect code (but we can contribute back to the project and fix it). |
|
@wangmingliang-ms - Would you like to do a PR for these changes? Thanks! Jon |
sure, I am happy to do that. |
|
Thanks you! |
Hi, @jongio here is the PR: #1122, it also fixes some other issues in addition to the above. |

This PR adds SimpleTodo templates for Java AppService and Container Apps. Fixes #415. Note: Java support was added in
azdwith #773.Manually tested:
azd up, monitor, downcommandsThanks @jdubois and @brunoborges for the initial SimpleTodo API implementation.