Skip to content

keystone: add v3 OS-FEDERATION extension List Mappings#2468

Merged
mandre merged 3 commits intogophercloud:masterfrom
OpenSource-THG:keystone-federation-mappings-list
Oct 13, 2022
Merged

keystone: add v3 OS-FEDERATION extension List Mappings#2468
mandre merged 3 commits intogophercloud:masterfrom
OpenSource-THG:keystone-federation-mappings-list

Conversation

@emilmaruszczak
Copy link
Copy Markdown
Contributor

@emilmaruszczak emilmaruszczak commented Sep 8, 2022

For #2461

docs
code

After this is reviewed and merged other operations will follow.

@mandre
Copy link
Copy Markdown
Contributor

mandre commented Sep 28, 2022

I took the liberty to modify your PR description so that it's not going to close #2461 automatically upon merging this PR, as it appears there are more work needed for it.

@emilmaruszczak emilmaruszczak force-pushed the keystone-federation-mappings-list branch from cae0d6f to aeccb45 Compare October 4, 2022 16:14
@emilmaruszczak
Copy link
Copy Markdown
Contributor Author

@mandre
I've added the acceptance test, hope it is enought to get this merged.

mappings, err := federation.ExtractMappings(allPages)
th.AssertNoErr(t, err)

tools.PrintResource(t, mappings)
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.

Is it expected mappings is null here?

Don't you need elevated privileges to list the federation mappings? In which case, I would have expected the call to return a 403 error. Not sure if this is a general issue with pagination in gophercloud or if this is related to this patch as I'm not really familiar about how the pagination works...

Also, we should probably be testing the mapping list after we have created a mapping first so that we confirm it works as expected.

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, as long as (pagination.LinkedPageBase).ExtractInto assigns nil as an empty array.

I can create an acceptance test for CRUD (as I've seen in other sets) for create operation and verify that it works as expected. Is it fine with you to leave it as is?

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.

That's fine for now, I expect we call again ListMappings when implementing the acceptance tests for the mapping creation.

client, err := clients.NewIdentityV3Client()
th.AssertNoErr(t, err)

allPages, err := federation.ListMappings(client).AllPages()
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.

Please run go fmt on acceptance/openstack/identity/v3/federation_test.go. This is the reason why the unit tests fail.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 80.033% when pulling 4b28068 on OpenSource-THG:keystone-federation-mappings-list into 9fb3d84 on gophercloud:master.

Copy link
Copy Markdown
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.

LGTM

mappings, err := federation.ExtractMappings(allPages)
th.AssertNoErr(t, err)

tools.PrintResource(t, mappings)
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.

That's fine for now, I expect we call again ListMappings when implementing the acceptance tests for the mapping creation.

@mandre mandre merged commit 9119080 into gophercloud:master Oct 13, 2022
@pierreprinetti pierreprinetti added this to the v1.1.0 milestone Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants