You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "SURYAS1306 (via GitHub)" <gi...@apache.org> on 2026/02/09 14:28:20 UTC
[PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
SURYAS1306 opened a new pull request, #12615:
URL: https://github.com/apache/cloudstack/pull/12615
### Description
Backport of #12607 to the 4.20 branch.
This change fixes an issue where account deletion was blocked if the account
was listed as an administrator of projects that were already removed
(removed is not null).
The fix ensures that only active projects are considered when checking
whether an account manages projects, allowing deletion when only deleted
projects are associated.
Includes the same defensive check and unit test added in the main branch.
Rebased to 4.20 as requested by @DaanHoogland
Fixes #12601
### Types of changes
- [x] Bug fix (non-breaking change which fixes an issue)
- [x] Test (unit test code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-4351148735
> I see the simulator smoke tests are consistently failing with an access denied error during volume creation. Investigating whether the project-admin validation change introduced a permission regression.
Let me know when ready @SURYAS1306 , I’ll give it another go when done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3944554314
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-4648984193
@SURYAS1306 @sureshanaparti @DaanHoogland @winterhazel
As I have mentioned in https://github.com/apache/cloudstack/pull/12607#issuecomment-4216054380, this doesn't really fix the root cause of the problem, as it leaves inconsistent records in the DB.
The actual fix is also much simpler than the proposed solution, only requiring 2 line changes: one to prevent this inconsistency from happening in the future (which can be achieved by removing the line that equals the `userid` parameter to `null` in the `ProjectAccountSearch` search builder on `ProjectAccountDaoImpl`), and one to normalize the inconsistent records already present on the DB.
What to you guys think of this alternative solution?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "SURYAS1306 (via GitHub)" <gi...@apache.org>.
SURYAS1306 commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3877471048
Hi @sureshanaparti ,
The build issue was caused by incorrect mocks in AccountManagerImplTest.
I’ve updated the test to use the injected _projectAccountDao and _projectDao fields and pushed the fix.
Could you please re-run the workflows when convenient?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland closed pull request #12615: [4.20] Fix account deletion blocked by deleted project admin mappings
URL: https://github.com/apache/cloudstack/pull/12615
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-4342517594
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-4343475193
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3958685417
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3872760612
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 16761
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3879248346
@SURYAS1306 , a new error. please see https://github.com/apache/cloudstack/actions/runs/21867500572/job/63118078002?pr=12615#step:7:11366 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3957733975
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "SURYAS1306 (via GitHub)" <gi...@apache.org>.
SURYAS1306 commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3960690074
I see the simulator smoke tests are consistently failing with an access denied error during volume creation. Investigating whether the project-admin validation change introduced a permission regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "SURYAS1306 (via GitHub)" <gi...@apache.org>.
SURYAS1306 commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3872237966
Hi @DaanHoogland
This PR backports the fix from #12607 to the 4.20 branch.
It contains the same change and unit test as the main/4.22 fix, adjusted for the 4.20 codebase.
Please let me know if any additional changes are needed for the backport.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3872239991
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "Tonitzpp (via GitHub)" <gi...@apache.org>.
Tonitzpp commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-4690404573
@SURYAS1306 are you still working on this? I am interested in this fix. If there is no feedback from the author within a week, I intend to open a PR containing the simpler solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3957745728
@DaanHoogland 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3897682394
@SURYAS1306 , I happen to struggle with `AccountManagerImplTest` atm myself. You have unit test errors in it. Have a look at #12632 to see how I am trying to clean it up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "SURYAS1306 (via GitHub)" <gi...@apache.org>.
SURYAS1306 commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3903981410
Hi @DaanHoogland,
I took a look at PR #12632 as you suggested and updated the test to stop mocking getCurrentCallingAccount() and use CallContext instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3875703162
@SURYAS1306 please check the build error.
```
Error: /home/runner/work/cloudstack/cloudstack/server/src/test/java/com/cloud/user/AccountManagerImplTest.java:[1601,21] error: cannot find symbol
Error: symbol: variable projectAccountDao
Error: location: class AccountManagerImplTest
Error: /home/runner/work/cloudstack/cloudstack/server/src/test/java/com/cloud/user/AccountManagerImplTest.java:[1607,21] error: cannot find symbol
Error: symbol: variable projectDao
Error: location: class AccountManagerImplTest
Error: /home/runner/work/cloudstack/cloudstack/server/src/test/java/com/cloud/user/AccountManagerImplTest.java:[1610,8] error: cannot find symbol
Error: symbol: variable accountManager
Error: location: class AccountManagerImplTest
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3872128960
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/12615?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 4.15%. Comparing base ([`a0f35a1`](https://app.codecov.io/gh/apache/cloudstack/commit/a0f35a186d826a7acbd65041bba1c4d2db1ddbfc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d57e1f8`](https://app.codecov.io/gh/apache/cloudstack/commit/d57e1f8f9764a9d42a524fcd81e752187f5f2dd4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (a0f35a1) and HEAD (d57e1f8). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (a0f35a1) | HEAD (d57e1f8) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.20 #12615 +/- ##
=============================================
- Coverage 16.27% 4.15% -12.12%
=============================================
Files 5662 404 -5258
Lines 500033 32965 -467068
Branches 60717 5893 -54824
=============================================
- Hits 81368 1370 -79998
+ Misses 409591 31419 -378172
+ Partials 9074 176 -8898
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/12615/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/12615/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.15% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/12615/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/12615?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-4343482771
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#discussion_r2783268853
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -2110,16 +2110,33 @@ public boolean deleteUserAccount(long accountId) {
return deleteAccount(account, callerUserId, caller);
}
- protected void checkIfAccountManagesProjects(long accountId) {
- List<Long> managedProjectIds = _projectAccountDao.listAdministratedProjectIds(accountId);
- if (!CollectionUtils.isEmpty(managedProjectIds)) {
- throw new InvalidParameterValueException(String.format(
- "Unable to delete account [%s], because it manages the following project(s): %s. Please, remove the account from these projects or demote it to a regular project role first.",
- accountId, managedProjectIds
- ));
+protected void checkIfAccountManagesProjects(long accountId) {
+ List<Long> managedProjectIds = _projectAccountDao.listAdministratedProjectIds(accountId);
+
+ if (CollectionUtils.isEmpty(managedProjectIds)) {
+ return;
+ }
+
+ List<Long> activeManagedProjects = new ArrayList<>();
+
+ for (Long projectId : managedProjectIds) {
+ ProjectVO project = _projectDao.findById(projectId);
+ if (project != null && project.getRemoved() == null) {
+ activeManagedProjects.add(projectId);
}
}
+ if (!activeManagedProjects.isEmpty()) {
+ throw new InvalidParameterValueException(
+ String.format(
+ "Unable to delete account [%s], because it manages the following project(s): %s. Please, remove the account from these projects or demote it to a regular project role first.",
+ accountId, activeManagedProjects
+ )
+ );
+ }
+}
Review Comment:
```suggestion
protected void checkIfAccountManagesProjects(long accountId) {
List<Long> managedProjectIds = _projectAccountDao.listAdministratedProjectIds(accountId);
if (CollectionUtils.isEmpty(managedProjectIds)) {
return;
}
List<Long> activeManagedProjects = new ArrayList<>();
for (Long projectId : managedProjectIds) {
ProjectVO project = _projectDao.findById(projectId);
if (project != null && project.getRemoved() == null) {
activeManagedProjects.add(projectId);
}
}
if (!activeManagedProjects.isEmpty()) {
throw new InvalidParameterValueException(
String.format(
"Unable to delete account [%s], because it manages the following project(s): %s. Please, remove the account from these projects or demote it to a regular project role first.",
accountId, activeManagedProjects
));
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "SURYAS1306 (via GitHub)" <gi...@apache.org>.
SURYAS1306 commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3892285348
Hi @DaanHoogland,
I moved the one-time initialization to @BeforeClass and kept only a single @Before for per-test stubbing. This removes the lifecycle ordering issue between setup phases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "SURYAS1306 (via GitHub)" <gi...@apache.org>.
SURYAS1306 commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3891353938
Hi @DaanHoogland,
I’ve removed the manual Mockito.mock calls and now rely on @Mock injection.
If it still fails, I’ll try consolidating the two @Before methods next.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "SURYAS1306 (via GitHub)" <gi...@apache.org>.
SURYAS1306 commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3929600988
Hi @DaanHoogland,
Following your suggestion, I reviewed #12632 and updated the test to stop mocking `getCurrentCallingAccount()` and use `CallContext`.
However, the CI still fails in `AccountManagerImplTest` during the setup phase:
`AccountManagerImplTest.setUp:167 NotAMock – Argument passed to when() is not a mock`
From the logs, all failures occur before any assertions run (the previous test class passes), meaning the failure is caused by the test initialization itself rather than the account deletion change in this PR.
While debugging I noticed `AccountManagerImplTestBase` creates real instances that override Mockito-injected mocks, so `when(...)` sometimes executes on non-mock objects depending on initialization order.
I tried:
* removing manual mocks
* relying only on `@Mock` injection
* using `@BeforeClass`
* using `CallContext`
* avoiding `spy`
but the failures persist and appear related to the legacy Mockito lifecycle in this test rather than the change introduced here.
Since this is a 4.20 backport, would you prefer:
1. keeping the functional fix minimal and not refactoring the legacy test, or
2. aligning the test structure with the cleanup approach from #12632 first?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-4553094768
ping @SURYAS1306 , are you still looking at this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-4559851849
@SURYAS1306 also, please see https://github.com/apache/cloudstack/pull/12607#issuecomment-4216054380
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "Tonitzpp (via GitHub)" <gi...@apache.org>.
Tonitzpp commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-4751834574
Since the author did not provide any feedback, I opened PR #13456 containing the simpler solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3872298545
@DaanHoogland 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "SURYAS1306 (via GitHub)" <gi...@apache.org>.
SURYAS1306 commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3880073586
Hi @DaanHoogland,
I see the new failure is a Mockito NotAMockException caused by an incorrect @Spy and @InjectMocks annotation order in AccountManagerImplTest.
I’ve fixed this by applying @Spy before @InjectMocks on AccountManagerImpl and pushed the update.
I’m re-checking the remaining test failures locally and will follow up shortly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3891718932
> If it still fails, I’ll try consolidating the two @Before methods next.
Don't, one of them should be a @BeforeClass method!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#discussion_r2803056855
##########
server/src/test/java/com/cloud/user/AccountManagerImplTest.java:
##########
@@ -131,24 +148,21 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase {
@Mock
RoleService roleService;
+ @BeforeClass
Review Comment:
did you forget to import it @SURYAS1306 ?
```
[INFO] -------------------------------------------------------------
Error: /home/runner/work/cloudstack/cloudstack/server/src/test/java/com/cloud/user/AccountManagerImplTest.java:[151,5] error: cannot find symbol
symbol: class BeforeClass
location: class AccountManagerImplTest
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3889196287
> AccountManagerImplTest.beforeTest:164 NotMock Argument passed to when() is not a mock
> * Having two @before methods with no guaranteed execution order
One of them should probably be @Before and the other @BeforeClass
> * Or possibly overwriting mocks inside setUp() with manual Mockito.mock() calls
I think no explicit Mocks should have to be created. @InitMocks should take care of injections.
>
> Would you recommend consolidating the @before methods into one, or removing the manual Mockito.mock() calls and relying only on @mock annotations?
no and yes, see above.
> Since this test extends AccountManagetImplTestBase, I want to ensure I’m aligning with the existing pattern.
The existing pattern may have gone through Mockito upgrades and is not necessarily the best. @vishesh92 , could you have a look as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3958137588
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16941
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-4342534824
@DaanHoogland 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on code in PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#discussion_r3624895121
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -2100,14 +2100,30 @@ public boolean deleteUserAccount(long accountId) {
protected void checkIfAccountManagesProjects(long accountId) {
List<Long> managedProjectIds = _projectAccountDao.listAdministratedProjectIds(accountId);
- if (!CollectionUtils.isEmpty(managedProjectIds)) {
- throw new InvalidParameterValueException(String.format(
+
+ if (CollectionUtils.isEmpty(managedProjectIds)) {
+ return;
+ }
+
+ List<Long> activeManagedProjects = new ArrayList<>();
+
+ for (Long projectId : managedProjectIds) {
+ ProjectVO project = _projectDao.findById(projectId);
+ if (project != null && project.getRemoved() == null) {
+ activeManagedProjects.add(projectId);
+ }
+ }
+
+ if (!activeManagedProjects.isEmpty()) {
+ throw new InvalidParameterValueException(
Review Comment:
Nit: this got reformatted from a single-line `String.format(...)` call to an oddly-broken multi-line one, inconsistent with the rest of the file's usual single-line style for calls this length.
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -2100,14 +2100,30 @@ public boolean deleteUserAccount(long accountId) {
protected void checkIfAccountManagesProjects(long accountId) {
List<Long> managedProjectIds = _projectAccountDao.listAdministratedProjectIds(accountId);
- if (!CollectionUtils.isEmpty(managedProjectIds)) {
- throw new InvalidParameterValueException(String.format(
+
+ if (CollectionUtils.isEmpty(managedProjectIds)) {
+ return;
+ }
+
+ List<Long> activeManagedProjects = new ArrayList<>();
+
+ for (Long projectId : managedProjectIds) {
+ ProjectVO project = _projectDao.findById(projectId);
+ if (project != null && project.getRemoved() == null) {
+ activeManagedProjects.add(projectId);
+ }
+ }
+
+ if (!activeManagedProjects.isEmpty()) {
+ throw new InvalidParameterValueException(
+ String.format(
"Unable to delete account [%s], because it manages the following project(s): %s. Please, remove the account from these projects or demote it to a regular project role first.",
- accountId, managedProjectIds
+ accountId, activeManagedProjects
));
}
}
+
protected boolean isDeleteNeeded(AccountVO account, long accountId, Account caller) {
Review Comment:
Nit: extra blank line above this method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "SURYAS1306 (via GitHub)" <gi...@apache.org>.
SURYAS1306 commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3877894410
Hi @sureshanaparti
The previous build failure was due to missing imports (ProjectAccountDao, ProjectDao, InjectMocks) in AccountManagerImplTest.
I’ve added the required imports and pushed the fix in the latest commit.
Could you please re-run the workflows when possible?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "SURYAS1306 (via GitHub)" <gi...@apache.org>.
SURYAS1306 commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3885135311
Hi @DaanHoogland,
I’ve updated the spy initialization in AccountManagerImplTest to ensure proper injection order and pushed the fix.
This should address the NullPointerException seen in the previous run.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3885606554
sorry @SURYAS1306 , another lint/checkstyle error: https://github.com/apache/cloudstack/actions/runs/21910909861/job/63274656053?pr=12615#step:11:6378
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "SURYAS1306 (via GitHub)" <gi...@apache.org>.
SURYAS1306 commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3885885092
Thanks @DaanHoogland ,
Fixed the checkstyle issue (unused import) and pushed the update.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "SURYAS1306 (via GitHub)" <gi...@apache.org>.
SURYAS1306 commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3886569125
Hi @DaanHoogland,
I'm still seeing persistent NotMock errors in AccountManagerImplTest. All failures point to:
AccountManagerImplTest.beforeTest:164 NotMock
Argument passed to when() is not a mock
So far I’ve:
- Added the missing _projectDao mock
- Updated usages to match production field names
- Fixed the accountManager → accountManagerImpl typo
- Added Mockito.spy(accountManagerImpl) in setUp()
I suspect this might be due to:
- Having two @Before methods with no guaranteed execution order
- Or possibly overwriting mocks inside setUp() with manual Mockito.mock() calls
Would you recommend consolidating the @Before methods into one, or removing the manual Mockito.mock() calls and relying only on @Mock annotations?
Since this test extends AccountManagetImplTestBase, I want to ensure I’m aligning with the existing pattern.
I’ll update based on your feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3944158843
> Hi @DaanHoogland,
>
> Following your suggestion, I reviewed #12632 and updated the test to stop mocking `getCurrentCallingAccount()` and use `CallContext`.
>
> However, the CI still fails in `AccountManagerImplTest` during the setup phase:
>
> `AccountManagerImplTest.setUp:167 NotAMock – Argument passed to when() is not a mock`
>
> From the logs, all failures occur before any assertions run (the previous test class passes), meaning the failure is caused by the test initialization itself rather than the account deletion change in this PR.
>
> While debugging I noticed `AccountManagerImplTestBase` creates real instances that override Mockito-injected mocks, so `when(...)` sometimes executes on non-mock objects depending on initialization order.
>
> I tried:
>
> * removing manual mocks
> * relying only on `@Mock` injection
> * using `@BeforeClass`
> * using `CallContext`
> * avoiding `spy`
>
> but the failures persist and appear related to the legacy Mockito lifecycle in this test rather than the change introduced here.
>
> Since this is a 4.20 backport, would you prefer:
>
> 1. keeping the functional fix minimal and not refactoring the legacy test, or
> 2. aligning the test structure with the cleanup approach from [prevent user.uuid from being regenerated on each operation by reading it from the DB #12632](https://github.com/apache/cloudstack/pull/12632) first?
As a first attempt, try rebasing onto latest 4.20. Try minimising any changes as this branch compiles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "SURYAS1306 (via GitHub)" <gi...@apache.org>.
SURYAS1306 commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3954432829
Hi @DaanHoogland
Rebased on the latest 4.20, resolved conflicts, updated the tests to reflect the active-project validation logic, and verified the full test suite locally (all passing).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3958688416
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-4349448409
<b>[SF] Trillian test result (tid-15982)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 51650 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr12615-t15982-kvm-ol8.zip
Smoke tests completed. 137 look OK, 0 have errors, 4 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
all_test_human_readable_logs | `Skipped` | --- | test_human_readable_logs.py
all_test_image_store_object_migration | `Skipped` | --- | test_image_store_object_migration.py
all_test_import_unmanage_volumes | `Skipped` | --- | test_import_unmanage_volumes.py
all_test_internal_lb | `Skipped` | --- | test_internal_lb.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-4342964792
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17653
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [4.20] Fix account deletion blocked by deleted project admin mappings [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #12615:
URL: https://github.com/apache/cloudstack/pull/12615#issuecomment-3958935166
<b>[SF] Trillian Build Failed (tid-15524)<b/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org