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

Changes from 1 commit
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
1 change: 1 addition & 0 deletions test/integration/component/test_tags.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1756,6 +1756,7 @@ def test_15_project_tag(self):
listall=True,
resourceType='project',
resourceIds=project.id,
projectid=project.id,

@yadvr yadvr Jun 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?

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

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

@DaanHoogland DaanHoogland Jun 6, 2019

Copy link
Copy Markdown
Contributor Author

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.

@DaanHoogland DaanHoogland Jun 6, 2019

Copy link
Copy Markdown
Contributor Author

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

@yadvr yadvr Jun 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).

@DaanHoogland DaanHoogland Jun 6, 2019

Copy link
Copy Markdown
Contributor Author

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.

@DaanHoogland DaanHoogland Jun 6, 2019

Copy link
Copy Markdown
Contributor Author

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.

@DaanHoogland DaanHoogland Jun 6, 2019

Copy link
Copy Markdown
Contributor Author

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)
Expand Down