server: set template type to ROUTING or USER if template type is not specified when upload a template#12768
Conversation
…specified when upload a template
|
@DaanHoogland @vishesh92 @vladimirpetrov |
e6d952c to
9dd0695
Compare
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Sounds good, is type null leading to NPEs? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12768 +/- ##
============================================
- Coverage 17.60% 17.60% -0.01%
Complexity 15676 15676
============================================
Files 5918 5918
Lines 531667 531669 +2
Branches 65001 65002 +1
============================================
- Hits 93617 93616 -1
- Misses 427491 427493 +2
- Partials 10559 10560 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I think it does not lead to NPEs, but template/iso are not usable as the type is NULL. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 17039 |
|
@blueorangutan package |
There was a problem hiding this comment.
Pull request overview
Updates template upload/registration validation to default the template type to ROUTING or USER when not explicitly provided, and aligns existing DB records by backfilling vm_template.type when it is NULL.
Changes:
- Extend template-type defaulting logic to
GetUploadParamsForTemplateCmd. - Add permission validation/error for non-admin users requesting upload params for non-
USERtemplate types. - Backfill
cloud.vm_template.typetoUSERwhere it is currentlyNULL.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/src/main/java/com/cloud/template/TemplateManagerImpl.java | Default template type for upload-params requests and restrict non-admin usage for non-USER types. |
| engine/schema/src/main/resources/META-INF/db/schema-42200to42210.sql | Data migration to set vm_template.type to USER for existing NULL rows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
engine/schema/src/main/resources/META-INF/db/schema-42200to42210.sql
Outdated
Show resolved
Hide resolved
server/src/main/java/com/cloud/template/TemplateManagerImpl.java
Outdated
Show resolved
Hide resolved
…10.sql Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@blueoranguran package |
1 similar comment
|
@blueoranguran package |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17049 |
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15584)
|
vladimirpetrov
left a comment
There was a problem hiding this comment.
LGTM based on manual testing.
|
@blueorangutan test |
|
@vladimirpetrov a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15600)
|
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15617)
|
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
[SF] Trillian test result (tid-15757)
|
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if (cmd instanceof RegisterTemplateCmd || cmd instanceof GetUploadParamsForTemplateCmd) { | ||
| boolean isRouting = Boolean.TRUE.equals(isRoutingType); | ||
| templateType = (cmd instanceof RegisterVnfTemplateCmd) ? TemplateType.VNF : (isRouting ? TemplateType.ROUTING : TemplateType.USER); | ||
| } |
There was a problem hiding this comment.
validateTemplateType now handles GetUploadParamsForTemplateCmd and defaults the template type to USER/ROUTING when templateType is omitted. There are existing unit tests for this method in server/src/test/java/com/cloud/template/TemplateManagerImplTest.java, but none cover the GetUploadParamsForTemplateCmd path; please add tests for (1) default USER when neither templateType nor isrouting is provided, (2) default ROUTING when isrouting=true (admin), and (3) rejection for non-admin when the resolved/explicit type is not USER/VNF.
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17288 |
Description
This PR fixes #12763
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?