Skip to content

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gebamp
Copy link

@gebamp gebamp commented Mar 31, 2024

With this PR, we add API support for:

  • Adding a tag on a project.
  • Checking if a project has a tag.
  • Deleting a tag from a project.

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

@github-actions github-actions bot added the semver:minor Backwards-compatible change label Mar 31, 2024
@coveralls
Copy link

coveralls commented Mar 31, 2024

Coverage Status

coverage: 78.749% (+0.02%) from 78.726%
when pulling 0668ddb on gebamp:identity-v3-project-tag
into 22fe3c1 on gophercloud:master.

@gebamp gebamp changed the title [wip] Add project tag functionalities in identity v3 Add project tag functionalities in identity v3 Mar 31, 2024
@gebamp
Copy link
Author

gebamp commented Apr 2, 2024

Hey, @mandre / @pierreprinetti could you possibly have a look at this? We sort of need it.

@pierreprinetti
Copy link
Member

@mandre do we ever use an admin account of devstack in the acceptance tests? This could be a nice candidate

Copy link
Contributor

@mandre mandre left a 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) {
Copy link
Contributor

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

Copy link
Author

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.

Copy link
Contributor

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?

Copy link
Author

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) {
Copy link
Contributor

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.

Copy link
Author

@gebamp gebamp Apr 8, 2024

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.

@github-actions github-actions bot added semver:major Breaking change and removed semver:minor Backwards-compatible change labels Apr 17, 2024
@gebamp
Copy link
Author

gebamp commented Apr 17, 2024

@mandre I did the Fixups, and included the acceptances tests!

@github-actions github-actions bot added semver:major Breaking change and removed semver:major Breaking change labels Apr 19, 2024
@gebamp
Copy link
Author

gebamp commented Apr 30, 2024

@mandre Can you check this out, when you have time?

@mandre
Copy link
Contributor

mandre commented Apr 30, 2024

Apologies @gebamp, I'm traveling and won't be able to do a proper review. @pierreprinetti @EmilienM could you take a look?

@EmilienM
Copy link
Contributor

EmilienM commented May 6, 2024

please rebase and then LGTM

@gebamp gebamp force-pushed the identity-v3-project-tag branch from 9f426b1 to 713cad7 Compare May 7, 2024 10:33
@github-actions github-actions bot added semver:minor Backwards-compatible change and removed semver:unknown labels May 7, 2024
@gebamp
Copy link
Author

gebamp commented May 7, 2024

@EmilienM Done with the rebase, when you have time feel free to merge

EmilienM
EmilienM previously approved these changes Jun 7, 2024
@EmilienM
Copy link
Contributor

EmilienM commented Jun 7, 2024

since @pierreprinetti and @mandre were involved too, I'll let them a chance to a last review. Thanks for your work!

@gebamp
Copy link
Author

gebamp commented Sep 25, 2024

@EmilienM @mandre @pierreprinetti It has been a couple of months, since the last comment. Do we have any updates wrt to this MR?

kayrus
kayrus previously approved these changes Sep 25, 2024
@kayrus
Copy link
Contributor

kayrus commented Sep 25, 2024

There is a hold lock in check that prevents this PR to be merged. Any idea how to fix it?

@gebamp
Copy link
Author

gebamp commented Sep 25, 2024

Nope, I havent seen this before :)

Copy link
Contributor

@kayrus kayrus left a 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.

Comment on lines 179 to 180
res := projects.CheckTag(context.TODO(), client.ServiceClient(), projectID, tag)
th.AssertNoErr(t, res.Err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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)

Copy link
Author

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

sure

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 190 to 191
res := projects.AddTag(context.TODO(), client.ServiceClient(), projectID, tag)
th.AssertNoErr(t, res.Err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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)

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 201 to 202
res := projects.DeleteTag(context.TODO(), client.ServiceClient(), projectID, tag)
th.AssertNoErr(t, res.Err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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)

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@pierreprinetti pierreprinetti added the hold Do not merge label Sep 25, 2024

// GetProjectTag is the result of a Get tag request. Call its Extract method to
// interpret it.
type CheckTagResult struct {
Copy link
Member

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

Comment on lines 210 to 211
// AddProjectTagResult is the result of a Delete request. Call its ExtractErr method to
// determine if the request succeeded or failed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// 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.

Copy link
Author

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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// 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

Copy link
Author

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},
Copy link
Member

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

Copy link
Author

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()
Copy link
Member

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.

Copy link
Author

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.

@pierreprinetti pierreprinetti added needinfo Additional information requested and removed hold Do not merge labels Sep 25, 2024
@github-actions github-actions bot added the edit:identity This PR updates identity code label Oct 21, 2024
@gebamp gebamp dismissed stale reviews from kayrus and EmilienM via 0668ddb October 21, 2024 13:11
@pierreprinetti
Copy link
Member

I am adding a HOLD label to signify that squashing is needed before merging.

@pierreprinetti pierreprinetti added hold Do not merge backport-v2 This PR will be backported to v2 and removed needinfo Additional information requested labels Oct 21, 2024
@gebamp
Copy link
Author

gebamp commented Oct 21, 2024

The only thing that is left is having the CheckTagResult option implement it's own Extract method.
If this is still required, I will go ahead and implement it adding new fixups. @pierreprinetti What is your opinion?

@pierreprinetti
Copy link
Member

The only thing that is left is having the CheckTagResult option implement it's own Extract method. If this is still required, I will go ahead and implement it adding new fixups. @pierreprinetti What is your opinion?

I still think that CheckTagResult should have an Extract method. Do you think differently?

@gebamp
Copy link
Author

gebamp commented Oct 23, 2024

The only thing that is left is having the CheckTagResult option implement it's own Extract method. If this is still required, I will go ahead and implement it adding new fixups. @pierreprinetti What is your opinion?

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.
However, for this to happen we need to be strict and remove 404 from the list of ok codes of the CheckTag method.
Personally, I think it would be better to keep only 204 as the only success code to follow the api reference and not to include any Extract method for the CheckTagResult.

A link to the api reference for the checktag method:
https://docs.openstack.org/api-ref/identity/v3/#check-if-project-contains-tag

@gebamp
Copy link
Author

gebamp commented Nov 20, 2024

@pierreprinetti Any news wrt this?

@pierreprinetti
Copy link
Member

The purpose of CheckTag is to tell whether the tag is present. The user shouldn't need to check the return code of the HTTP call to get their answer.

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 Extract() method on CheckTagResult that returns (bool, error) and leave both 204 and 404 as accepted codes, as they literally encode the answer to the question on that API endpoint.

	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")
	}

@gebamp
Copy link
Author

gebamp commented Nov 29, 2024

@pierreprinetti Thank you for the feedback, I will go ahead update the code based on the above requests!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v2 This PR will be backported to v2 edit:identity This PR updates identity code hold Do not merge semver:minor Backwards-compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants