-
Notifications
You must be signed in to change notification settings - Fork 559
Add project tag functionalities in identity v3 #3018
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.
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
base: main
Are you sure you want to change the base?
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@mandre I did the Fixups, and included the acceptances tests! |
@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 |
2d52089
to
9f426b1
Compare
9f426b1
to
713cad7
Compare
@EmilienM Done with the rebase, when you have time feel free to merge |
since @pierreprinetti and @mandre were involved too, I'll let them a chance to a last review. Thanks for your work! |
@EmilienM @mandre @pierreprinetti It has been a couple of months, since the last comment. Do we have any updates wrt to this MR? |
There is a |
Nope, I havent seen this before :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not critical, but would be nice to have this as well.
res := projects.CheckTag(context.TODO(), client.ServiceClient(), projectID, tag) | ||
th.AssertNoErr(t, res.Err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res := projects.CheckTag(context.TODO(), client.ServiceClient(), projectID, tag) | |
th.AssertNoErr(t, res.Err) | |
err := projects.CheckTag(context.TODO(), client.ServiceClient(), projectID, tag).ExtractErr() | |
th.AssertNoErr(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to move forward without this, as I cant pick it up right now? (same applies for the rest)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
res := projects.AddTag(context.TODO(), client.ServiceClient(), projectID, tag) | ||
th.AssertNoErr(t, res.Err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res := projects.AddTag(context.TODO(), client.ServiceClient(), projectID, tag) | |
th.AssertNoErr(t, res.Err) | |
err := projects.AddTag(context.TODO(), client.ServiceClient(), projectID, tag).ExtractErr() | |
th.AssertNoErr(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
res := projects.DeleteTag(context.TODO(), client.ServiceClient(), projectID, tag) | ||
th.AssertNoErr(t, res.Err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res := projects.DeleteTag(context.TODO(), client.ServiceClient(), projectID, tag) | |
th.AssertNoErr(t, res.Err) | |
err := projects.DeleteTag(context.TODO(), client.ServiceClient(), projectID, tag).ExtractErr() | |
th.AssertNoErr(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
// GetProjectTag is the result of a Get tag request. Call its Extract method to | ||
// interpret it. | ||
type CheckTagResult struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be cool to have CheckTagResult
implement an Extract
method that returns a bool and an error? A 404 should result in (false, nil)
.
// AddProjectTagResult is the result of a Delete request. Call its ExtractErr method to | ||
// determine if the request succeeded or failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// AddProjectTagResult is the result of a Delete request. Call its ExtractErr method to | |
// determine if the request succeeded or failed. | |
// AddTagResult is the result of an AddTag request. Call its ExtractErr method to | |
// determine whether the request succeeded or failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -200,3 +200,21 @@ func (r ModifyTagsResult) Extract() (*ProjectTags, error) { | |||
type DeleteTagsResult struct { | |||
gophercloud.ErrResult | |||
} | |||
|
|||
// GetProjectTag is the result of a Get tag request. Call its Extract method to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// GetProjectTag is the result of a Get tag request. Call its Extract method to | |
// CheckTagResult is the result of a CheckTag request. Call its Extract method to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
// CheckTag checks if a a tag exists in a project. | ||
func CheckTag(ctx context.Context, client *gophercloud.ServiceClient, projectID string, tag string) (r CheckTagResult) { | ||
resp, err := client.Get(ctx, getTagURL(client, projectID, tag), nil, &gophercloud.RequestOpts{ | ||
OkCodes: []int{204}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that 404 should be an OK code here. It's a legit response and it doesn't indicate a failure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
404 was added in the list of OkCodes
err = projects.CheckTag(context.TODO(), client, projectMain.ID, "Tag1").ExtractErr() | ||
th.AssertNoErr(t, err) | ||
|
||
err = projects.DeleteTag(context.TODO(), client, projectMain.ID, "Tag1").ExtractErr() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test sequence! It would be nice to run CheckTag
here below to verify that it's gone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the extra check tag at the end.
I am adding a HOLD label to signify that squashing is needed before merging. |
The only thing that is left is having the CheckTagResult option implement it's own Extract method. |
I still think that CheckTagResult should have an Extract method. Do you think differently? |
Since the user, can infer the result from the return codes of the error using the ExtractErr method, I dont think this is needed. A link to the api reference for the checktag method: |
@pierreprinetti Any news wrt this? |
The purpose of I don't think we want this code on the caller side: checkTagResult := projects.CheckTag(ctx, identityClient, "myproject", "mytag")
if err := checkTagResult.ExtractErr(); err != nil {
if checkTagResult.Result.StatusCode == 404 {
fmt.Print("The tag does not exist")
} else {
panic(err)
}
} else {
fmt.Print("The tag exists")
} Please implement an exists, err := projects.CheckTag(ctx, identityClient, "myproject", "mytag").Extract()
if err != nil {
panic(err)
}
if exists {
fmt.Print("The tag exists")
} else {
fmt.Print("The tag does not exist")
} |
@pierreprinetti Thank you for the feedback, I will go ahead update the code based on the above requests! |
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