project id added in test #3378

DaanHoogland · 2019-06-05T13:38:41Z

as per @ustcweizhou's comment: #3377 (comment)

Description

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?

@ustcweizhou

as per @ustcweizhou's comment: #3377 (comment)

ustcweizhou · 2019-06-05T20:35:30Z

I saved test_15_project_tag in a seperated python file, and run it in marvin test
it failed without this change, and succeed with this change.

so this is ok for me. @DaanHoogland @rhtyd

rohityadavcloud · 2019-06-06T07:03:34Z

test/integration/component/test_tags.py

@@ -1756,6 +1756,7 @@ def test_15_project_tag(self):
            listall=True,
            resourceType='project',
            resourceIds=project.id,
+            projectid=project.id,


I would have expected this to work without specifying project ID. Do you think we need to fix mgmt server instead?

@DaanHoogland Read this https://github.com/apache/cloudstack/pull/3323/files, how about we add a check to read the resource type and use the provided resource ID as account ID when type is project.

@rhtyd that should be good.

yes, that would work, @rhtyd. I don't think it matters much, as the id is already in the API, but that solution would have less breakage of ecosystem scripting so let's go for it.

on second thought @rhtyd , you do mean to use the resource ID to get the account ID and not use it as account ID, right? (else I don't understand 😮 )

yes @DaanHoogland, I'm more concerned with existing systems and integrations that may break. And moreover, it breaks the API semantics (we're already passing projectid in the resource ID so why not use it).

on second thought @rhtyd , it won't work. There is no resource ID unless the project ID is passed as such. in the call there is no information that we can use

listall=True, resourceType='project', key=tag_key,

We could get the current account from the call context but that would only satisfy some use-cases. if an admin wants tags for some project they are not in we still need the parameter.

never mind, i think I was stupid just there ^^. I'll revisit.

I've been mostly busy on other things today but gave it a second look. I think it can be fixed as you suggest in the query service. I will give it a swing soon. I'll remove the java change for now but leave the test in.

DaanHoogland · 2019-06-06T08:25:25Z

I think I just overkilled it. didn't want to spend more time on it.

DaanHoogland · 2019-06-06T09:04:16Z

@ustcweizhou I realize that now. I am confused by @rhtyd 's remark to use the resource ID it would not be the account ID of the owner of the project but the project itself. I don't think we can use that either. We could use it to find the account ID.

DaanHoogland · 2019-06-07T09:01:00Z

note to self; need to do something like:

        Long projectId = cmd.getProjectId() == null ? cmd.getResourceId() : cmd.getProjectId();
        
        _accountMgr.buildACLSearchParameters(caller, null, cmd.getAccountName(), projectId, permittedAccounts, domainIdRecursiveListProject, listAll, false);

but translate cmd.getResourceId() to a Long id (from uuid?)

ustcweizhou · 2019-06-07T09:28:53Z

getProjectId

@DaanHoogland
looked into the code, resourceId can be id or uuid, both are supported.

ustcweizhou · 2019-06-13T09:30:12Z

server/src/main/java/com/cloud/api/query/QueryManagerImpl.java

@@ -646,7 +646,9 @@

        Ternary<Long, Boolean, ListProjectResourcesCriteria> domainIdRecursiveListProject = new Ternary<Long, Boolean, ListProjectResourcesCriteria>(cmd.getDomainId(), cmd.isRecursive(), null);

-        _accountMgr.buildACLSearchParameters(caller, null, cmd.getAccountName(), cmd.getProjectId(), permittedAccounts, domainIdRecursiveListProject, listAll, false);
+        Long projectId = cmd.getProjectId() == null ? cmd.getResourceId() : cmd.getProjectId();


public String getResourceId() { return resourceId; }

anuragaw

Previous approval was based on 1 line change. Need to review again after mutiple changes.

anuragaw

Previous approval was based on 1 line change. Need to review again after new changes.

server/src/main/java/com/cloud/api/query/QueryManagerImpl.java

rohityadavcloud · 2019-06-20T17:57:33Z

@DaanHoogland failed following marvin test: (not sure if it's related to the PR)

test_15_project_tag     | marvin.cloudstackExcept | 1.249   | test_tags      |
|                         | ion.CloudstackAPIExcept |         |                |
|                         | ion                     |         |

ustcweizhou · 2019-06-20T20:55:31Z

server/src/main/java/com/cloud/api/query/QueryManagerImpl.java

+            try {
+                projectId = Long.parseLong(resourceId);
+            } catch (NumberFormatException e) {
+                projectId = _projectDao.findByUuid(resourceId).getId();


What if the uuid is invalid ?

it throws an exception: garbage in, garbage out. should we handle that? I think it is the exception @rhtyd is pointing at. How do you suggest this should be handled?

rohityadavcloud · 2019-07-03T06:36:10Z

Ping @DaanHoogland

rohityadavcloud · 2019-07-08T09:40:48Z

I'm taking over the failing smoketests/Travis issues here: #3476
Closing on this remark.

project id added in test

10a29c5

as per @ustcweizhou's comment: #3377 (comment)

anuragaw approved these changes Jun 6, 2019

View reviewed changes

rohityadavcloud reviewed Jun 6, 2019

View reviewed changes

backwards compatible project id handling

a39505c

DaanHoogland added 3 commits June 6, 2019 16:11

Update TaggedResourceManagerImpl.java

a478704

Update TaggedResourceManagerImpl.java

7295623

Update test_tags.py

b39d3c4

rohityadavcloud mentioned this pull request Jun 7, 2019

Enable service offerings to be scoped to domain(s) and zone(s) #3248

Merged

5 tasks

DaanHoogland added 2 commits June 13, 2019 10:23

make resourceId be used as projectId if the latter is missing

2fcbb13

space

8462fce

ustcweizhou reviewed Jun 13, 2019

View reviewed changes

Update QueryManagerImpl.java

b47ae14

anuragaw approved these changes Jun 13, 2019

View reviewed changes

anuragaw suggested changes Jun 13, 2019

View reviewed changes

ustcweizhou reviewed Jun 13, 2019

View reviewed changes

server/src/main/java/com/cloud/api/query/QueryManagerImpl.java Outdated Show resolved Hide resolved

Update QueryManagerImpl.java

e1f5f7a

DaanHoogland added the status:needs-work label Jun 18, 2019

rohityadavcloud added this to the 4.13.0.0 milestone Jun 20, 2019

ustcweizhou reviewed Jun 20, 2019

View reviewed changes

rohityadavcloud closed this Jul 8, 2019

rohityadavcloud removed this from the 4.13.0.0 milestone Jul 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

project id added in test #3378

project id added in test #3378

DaanHoogland commented Jun 5, 2019

ustcweizhou commented Jun 5, 2019

rohityadavcloud Jun 6, 2019

rohityadavcloud Jun 6, 2019

ustcweizhou Jun 6, 2019

DaanHoogland Jun 6, 2019

DaanHoogland Jun 6, 2019

rohityadavcloud Jun 6, 2019

DaanHoogland Jun 6, 2019

DaanHoogland Jun 6, 2019

DaanHoogland Jun 6, 2019

DaanHoogland commented Jun 6, 2019

DaanHoogland commented Jun 6, 2019

DaanHoogland commented Jun 7, 2019

ustcweizhou commented Jun 7, 2019

ustcweizhou Jun 13, 2019

anuragaw left a comment

anuragaw left a comment

rohityadavcloud commented Jun 20, 2019

ustcweizhou Jun 20, 2019

DaanHoogland Jul 3, 2019

rohityadavcloud commented Jul 3, 2019

rohityadavcloud commented Jul 8, 2019

project id added in test #3378

project id added in test #3378

Conversation

DaanHoogland commented Jun 5, 2019

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

ustcweizhou commented Jun 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaanHoogland commented Jun 6, 2019

DaanHoogland commented Jun 6, 2019

DaanHoogland commented Jun 7, 2019

ustcweizhou commented Jun 7, 2019

Choose a reason for hiding this comment

anuragaw left a comment

Choose a reason for hiding this comment

anuragaw left a comment

Choose a reason for hiding this comment

rohityadavcloud commented Jun 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohityadavcloud commented Jul 3, 2019

rohityadavcloud commented Jul 8, 2019