Add project tag functionalities in identity v3 #3018
Conversation
gebamp
changed the title
|
Hey, @mandre / @pierreprinetti could you possibly have a look at this? We sort of need it. |
|
@mandre do we ever use an admin account of devstack in the acceptance tests? This could be a nice candidate |
There was a problem hiding this comment.
I'm not exactly clear on how this differs from tag management via the /v3/projects endpoint, but eh... we're not here to discuss OpenStack API.
I have a few comments about naming, and arguments passed to the functions. It would also be nice if we implemented the ListTags() API call as well.
Could you make sure to add acceptance tests as well so that we're exercising this new code against real OpenStack deployments, for all supported version of OpenStack?
| } | ||
|
|
||
| // AddProjectTag adds a tag in a project. | ||
| func AddProjectTag(ctx context.Context, client *gophercloud.ServiceClient, projectID string, tag string) (r AddProjectTagResult) { |
There was a problem hiding this comment.
This should take a list of tags. Also, drop Project from the function name and make this plural. This should then be called AddTags().
There was a problem hiding this comment.
Regarding this, we are implementing Add single tag to a project which only takes one tag , and not Modify tag list for a project which takes a list of tags. Therefore, I think the name AddTag() is more suitable and by extension I dont think we need to change the tag argument to a list.
There was a problem hiding this comment.
Yeah, you're right. I've somehow missed the API call that takes a tag. That being said, it shouldn't be difficult to add the variants that take a list of tags and perhaps it would be worth it to add it there too. Same for the variant of Delete that drops all tags. What do you think?
There was a problem hiding this comment.
I agree that it wouldn't be too much of a hustle to add the list and delete all tags API calls. But, I would rather adding the rest of the project tag of the API suite in another PR, because we need the three API calls of this MR kinda quickly.
| } | ||
|
|
||
| // DeleteProjectTag deletes a tag from a project. | ||
| func DeleteProjectTag(ctx context.Context, client *gophercloud.ServiceClient, projectID string, tag string) (r DeleteProjectTagResult) { |
There was a problem hiding this comment.
Again, make this plural -> DeleteTags().
Also, the API call drops all tags, and should not take a tag argument.
There was a problem hiding this comment.
For this one, we want to implement the Delete a single tag from project API call which takes a single tag and deletes it and not the Remove all tags from a project which drops all tags. Therefore, DeleteTag is a more suitable name, and therefore I think we still need the tag argument.
github-actions
bot
added
semver:major
|
@mandre I did the Fixups, and included the acceptances tests! |
github-actions
bot
added
semver:major
|
@mandre Can you check this out, when you have time? |
|
Apologies @gebamp, I'm traveling and won't be able to do a proper review. @pierreprinetti @EmilienM could you take a look? |
|
please rebase and then LGTM |
github-actions
bot
added
semver:unknown
gebamp
force-pushed
the
identity-v3-project-tag
branch
from
2d52089
to
9f426b1
Compare
github-actions
bot
added
semver:unknown
gebamp
force-pushed
the
identity-v3-project-tag
branch
from
9f426b1
to
713cad7
Compare
github-actions
bot
added
semver:minor
|
@EmilienM Done with the rebase, when you have time feel free to merge |
With this PR, we add API support for:
Partially implements: #3015
Check if project contains tag:
API Docs
Py code
Add tag to project:
API Docs
Py code
Delete a single tag from project:
API Docs
Py code