FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Set isrecursive to false when listall=true by default for users by davidjumani · Pull Request #6111 · apache/cloudstack · GitHub

  1. apache
  2. cloudstack

Repository navigation

Set isrecursive to false when listall=true by default for users - #6111

Closed
davidjumani wants to merge 3 commits into
apache:mainfrom
shapeblue:fix-isrecursive-listoffering
ConversationCommits (3)ChecksFiles changed
Closed

Set isrecursive to false when listall=true by default for users#6111
davidjumani wants to merge 3 commits into
apache:mainfrom
shapeblue:fix-isrecursive-listoffering

Conversation

davidjumani commented Mar 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Sets isrecursive=false for users when listall=true by default to prevent any permission issues when listing resources as a user

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Screenshots (if appropriate):

How Has This Been Tested?

nvazquez added this to the 4.17.0.0 milestone Mar 14, 2022

nvazquez commented Mar 14, 2022

Copy link
Copy Markdown
Contributor

@blueorangutan package

blueorangutan commented Mar 14, 2022

Copy link
Copy Markdown

@nvazquez a 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.

blueorangutan commented Mar 14, 2022

Copy link
Copy Markdown

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2870

nvazquez commented Mar 14, 2022

Copy link
Copy Markdown
Contributor

@blueorangutan test

blueorangutan commented Mar 14, 2022

Copy link
Copy Markdown

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan commented Mar 15, 2022

Copy link
Copy Markdown

Trillian test result (tid-3599)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 29067 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6111-t3599-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py

davidjumani commented Mar 15, 2022

Copy link
Copy Markdown
Contributor Author

@blueorangutan test

blueorangutan commented Mar 15, 2022

Copy link
Copy Markdown

@davidjumani a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

davidjumani commented Mar 15, 2022

Copy link
Copy Markdown
Contributor Author

@blueorangutan package

blueorangutan commented Mar 15, 2022

Copy link
Copy Markdown

@davidjumani a 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.

davidjumani changed the title Fix recursive lookup for offerings for users Set isrecursive to false when listall=true by default for users Mar 15, 2022
nvazquez approved these changes Mar 15, 2022
View reviewed changes

nvazquez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Code LGTM

blueorangutan commented Mar 15, 2022

Copy link
Copy Markdown

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2885

nvazquez commented Mar 15, 2022

Copy link
Copy Markdown
Contributor

@blueorangutan test

blueorangutan commented Mar 15, 2022

Copy link
Copy Markdown

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

sureshanaparti approved these changes Mar 15, 2022
View reviewed changes

sureshanaparti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

code LGTM

Pearl1594 reviewed Mar 15, 2022
View reviewed changes
Comment thread api/src/main/java/org/apache/cloudstack/api/BaseListDomainResourcesCmd.java
return recursive == null ? true : recursive;
Account caller = CallContext.current().getCallingAccount();
if (caller.getType() != Account.Type.NORMAL) {
return recursive == null ? true : recursive;

Pearl1594 Mar 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could be simplified to

Suggested change
return recursive == null ? true : recursive;
return recursive == null || recursive;

DaanHoogland Mar 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

more concise, less readable /m no me gusta

Pearl1594 reviewed Mar 15, 2022
View reviewed changes
Comment thread api/src/main/java/org/apache/cloudstack/api/BaseListDomainResourcesCmd.java
return recursive == null ? true : recursive;
}
}
return recursive == null ? false : recursive;

Pearl1594 Mar 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could probably be simplified to:

Suggested change
return recursive == null ? false : recursive;
return recursive != null && recursive;

DaanHoogland Mar 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

hm, that is not more expressive, i'd say.

Pearl1594 reviewed Mar 15, 2022
View reviewed changes

Pearl1594 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM - verified fix - Prior fix few APIs failed with the following execption:
Only ROOT admins and Domain admins can list service offerings with isrecursive=true

blueorangutan commented Mar 15, 2022

Copy link
Copy Markdown

Trillian test result (tid-3613)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34034 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6111-t3613-kvm-centos7.zip
Smoke tests completed. 87 look OK, 5 have errors
Only failed tests results shown below:

Test Result Time (s) Test File
test_01_add_primary_storage_disabled_host Error 0.63 test_primary_storage.py
test_01_primary_storage_nfs Error 0.11 test_primary_storage.py
ContextSuite context=TestStorageTags>:setup Error 0.19 test_primary_storage.py
test_03_deploy_and_scale_kubernetes_cluster Failure 29.86 test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster Failure 60.50 test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster Failure 40.13 test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster Failure 0.05 test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown Error 78.75 test_kubernetes_clusters.py
test_01_secure_vm_migration Error 151.49 test_vm_life_cycle.py
test_02_unsecure_vm_migration Error 267.94 test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration Error 142.87 test_vm_life_cycle.py
test_08_migrate_vm Error 43.70 test_vm_life_cycle.py
test_02_list_snapshots_with_removed_data_store Error 9.43 test_snapshots.py
test_02_list_snapshots_with_removed_data_store Error 9.44 test_snapshots.py
test_hostha_enable_ha_when_host_in_maintenance Error 303.73 test_hostha_kvm.py

davidjumani commented Mar 16, 2022

Copy link
Copy Markdown
Contributor Author

@blueorangutan test

blueorangutan commented Mar 16, 2022

Copy link
Copy Markdown

@davidjumani a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

davidjumani commented Mar 16, 2022

Copy link
Copy Markdown
Contributor Author

Closing in favour of #6125

yadvr removed this from the 4.17.0.0 milestone Mar 24, 2022
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

DaanHoogland DaanHoogland left review comments
Pearl1594 Pearl1594 left review comments
nvazquez nvazquez approved these changes
sureshanaparti sureshanaparti approved these changes
weizhouapache Awaiting requested review from weizhouapache

Assignees

No one assigned

Labels

component:api

Projects

No open projects
Apache CloudStack 4.17.0
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL