Skip to content

project id added in test#3378

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.

Sign up for GitHub

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

Closed
DaanHoogland wants to merge 9 commits into from
Closed

project id added in test #3378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
10a29c5
project id added in test
DaanHoogland Jun 5, 2019
a39505c
backwards compatible project id handling
DaanHoogland Jun 6, 2019
a478704
Update TaggedResourceManagerImpl.java
DaanHoogland Jun 6, 2019
7295623
Update TaggedResourceManagerImpl.java
DaanHoogland Jun 6, 2019
b39d3c4
Update test_tags.py
DaanHoogland Jun 6, 2019
2fcbb13
make resourceId be used as projectId if the latter is missing
DaanHoogland Jun 13, 2019
8462fce
space
DaanHoogland Jun 13, 2019
b47ae14
Update QueryManagerImpl.java
DaanHoogland Jun 13, 2019
e1f5f7a
Update QueryManagerImpl.java
DaanHoogland Jun 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion server/src/main/java/com/cloud/api/query/QueryManagerImpl.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -646,7 +646,16 @@ private Pair<List<ResourceTagJoinVO>, Integer> listTagsInternal(ListTagsCmd cmd)

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();
if (resourceType.equalsIgnoreCase("project") && projectId == null) {
try {
projectId = Long.parseLong(resourceId);
} catch (NumberFormatException e) {
projectId = _projectDao.findByUuid(resourceId).getId();

@ustcweizhouustcweizhouJun 20, 2019

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.

What if the uuid is invalid ?

@DaanHooglandDaanHooglandJul 3, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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?

}
}
_accountMgr.buildACLSearchParameters(caller, null, cmd.getAccountName(), projectId, permittedAccounts, domainIdRecursiveListProject, listAll, false);

Long domainId = domainIdRecursiveListProject.first();
Boolean isRecursive = domainIdRecursiveListProject.second();
ListProjectResourcesCriteria listProjectResourcesCriteria = domainIdRecursiveListProject.third();
Expand Down
20 changes: 19 additions & 1 deletion test/integration/component/test_tags.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1755,21 +1755,39 @@ def test_15_project_tag(self):
self.apiclient,
listall=True,
resourceType='project',
resourceIds=project.id,
projectid=project.id,

@yadvryadvrJun 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@yadvryadvrJun 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@ustcweizhouustcweizhouJun 6, 2019

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.

@rhtyd that should be good.

@DaanHooglandDaanHooglandJun 6, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@DaanHooglandDaanHooglandJun 6, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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 😮 )

@yadvryadvrJun 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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).

@DaanHooglandDaanHooglandJun 6, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@DaanHooglandDaanHooglandJun 6, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

@DaanHooglandDaanHooglandJun 6, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

key=tag_key,
)
self.debug("tags = %s" % tags)
tag2 = Tag.list(
self.apiclient,
listall=True,
resourceType='project',
resourceId=project.id,
key=tag_key,
)
self.debug("tag2 = %s" % tags)

self.assertEqual(
isinstance(tags, list),
True,
"List tags should not return empty response"
)
self.assertEqual(
isinstance(tag2, list),
True,
"List tags should not return empty response"
)
self.assertEqual(
tags[0].value,
tag_value,
'The tag should have original value'
)
self.assertEqual(
tag2[0].value,
tag_value,
'The tag should have original value'
)

projects = Project.list(
self.apiclient,
Expand Down