Add AdminOrgVdcStorageProfile for querying #375

lelvisl · 2021-05-11T13:40:16Z

Issue: #373

Added:

QueryResultAdminOrgVdcStorageProfileRecordType type.
Constants for querying: QtOrgVdcStorageProfile and QtAdminOrgVdcStorageProfile.
Funcs for querying : QueryAdminOrgVdcStorageProfileByID and QueryOrgVdcStorageProfileByID.

QueryOrgVdcStorageProfileByID (non Admin) not workin (tested wiht vCloud 10.0 and 10.2.2).
May be query isn't correct.

Added: - QueryResultAdminOrgVdcStorageProfileRecordType type. - Constants for querying: QtOrgVdcStorageProfile and QtAdminOrgVdcStorageProfile. - Funcs for querying : QueryAdminOrgVdcStorageProfileByID and QueryOrgVdcStorageProfileByID. QueryOrgVdcStorageProfileByID (non Admin) not workin (tested wiht vCloud 10.0 and 10.2.2). May be query isn't correct.

vmwclabot · 2021-05-11T13:40:33Z

@lelvisl, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

vmwclabot · 2021-05-12T06:41:17Z

@lelvisl, VMware has approved your signed contributor license agreement.

lelvisl · 2021-05-14T17:03:54Z

QueryOrgVdcStorageProfileByID (non Admin) not workin (tested wiht vCloud 10.0 and 10.2.2) - wokring well if you are not provider admin.
As i think, this PR can be merged.

lelvisl · 2021-05-24T22:03:42Z

Can we merge this PR or we need to discuss something?

Didainius

Sorry for this drag. Going for review and testing.

Would you be able to add a line to CHANGELOG.md about this?
Also - any chances you could put a few tests into system_test.go (there are quite a few similar ones if you need inspiration :))

types/v56/constants.go

types/v56/types.go

- corrected review notes - added point to changelog - update test suit and add test

lelvisl · 2021-05-27T23:29:33Z

Thanks for your review! Can you look again please?

govcd/sample_govcd_test_config.yaml

Didainius · 2021-05-28T12:43:04Z

govcd/system.go

@@ -975,3 +975,29 @@ func (vcdCli *VCDClient) GetOrgList() (*types.OrgList, error) {
 	}
 	return orgList, nil
 }
+
+// QueryAdminOrgVdcStorageProfileByID finds a StorageProfile of VDC by ID as admin
+func QueryAdminOrgVdcStorageProfileByID(vcdCli *VCDClient, id string) ([]*types.QueryResultAdminOrgVdcStorageProfileRecordType, error) {


Thanks for this. Now just pardon, but I just tried to play with code and one thing hit me.
I think the signature could be:

Suggested change

func QueryAdminOrgVdcStorageProfileByID(vcdCli *VCDClient, id string) ([]*types.QueryResultAdminOrgVdcStorageProfileRecordType, error) {

func QueryAdminOrgVdcStorageProfileByID(vcdCli *VCDClient, id string) (*types.QueryResultAdminOrgVdcStorageProfileRecordType, error) {

That is - you don't need to return a slice of results because you are querying by ID which guarantees that there is either exactly one result, or not.

A slice could be if it was QueryOrgVdcStorageProfile which doesn't guarantee one object.

Can i then rename this method to GetAdminOrgVdcStorageProfileByID?

Or implement QueryOrgVdcStorageProfile with params and GetAdminOrgVdcStorageProfileByID that use previous?

GetX in this SDK usually mean that it is not an object from query endpoint, but from its legitimate endpoint (e.g. AdminOrg.GetVDCById would not return a Query result, but the VDC struct itself. Query and object endpoints usually return different data that is why we try to keep Query and Get (to differentiate these things). This is not 100% due to historic reasons, but that is a good thing to follow.

Or - if you want to implement a function that returns all objects then it could be QueryAdminOrgVdcStorageProfile. But if you simply leave the code as it is (and not apply ID filter) you will hit a problem - by default it returns only the first page of data (which is usually up to 25 items). It is not easy to spot this on testing envs where not many resources exist, but we have already hit that and fixed some of the problems here. The trick here is to use client.cumulativeQuery internally which handles pagination and makes sure all pages are retrieved. (You could look through functions like QueryEdgeGatewayList or QueryAllVdcs to see how it is implemented.)

Agree with you and changed signature.

About QueryAdminOrgVdcStorageProfile - i think we need separate issue for discuss.

Didainius · 2021-05-28T17:45:33Z

Not sure if you saw that, but I put a comment: #375 (comment)

…now returns only one vdcStorageProfile

Didainius

One last convenience spot from me and I agree QueryAdminOrgVdcStorageProfile can be left for later on.

govcd/system.go

…nOrgVdcStorageProfileByID

Didainius

Thanks. Approved and I left two typo comments and will wait for other team members to have a glimpse before we can merge.

govcd/system.go

lvirbalas

LGTM and thanks for contributing!

lelvisl · 2021-06-10T15:00:44Z

@dataclouder can you please review this PR?

govcd/system.go

dataclouder · 2021-06-11T13:45:24Z

Another issue: You should merge your PR with master, as some important pieces are missing. When you run the test for QueryOrgVdcStorageProfileByID, for example, you will need tenant credentials, and with the current code authentication will fail. (The bug was fixed in PR #376 )

…rofileRecord # Conflicts: # CHANGELOG.md

updated fields for QueryResultAdminOrgVdcStorageProfileRecordType and QueryResultOrgVdcStorageProfileRecordType add skip/error in case of Client.IsSysAdmin - vmware#384

lelvisl · 2021-06-12T12:18:46Z

@dataclouder i merged upstream into my brunch and updated all fields.

About tests:
Please look at #384. Test suit now is provider oriented, i can't (or don't know) how to setup both test successful from single user and i add skip in case of Client.IsSysAdmin (but it's not looking good for me).

lelvisl · 2021-09-01T18:17:49Z

How about this change? Close? Or you want this changes? If yes - please give some feedback.

As i see - PR #393 doesn't solve this problem

Didainius · 2022-08-18T11:49:39Z

@lelvisl, can you have a look at #499. We're finally hoping to resolve this and close. I merged up with latest, impoved tests.

Didainius reviewed May 26, 2021

View reviewed changes

types/v56/constants.go Outdated Show resolved Hide resolved

types/v56/types.go Outdated Show resolved Hide resolved

Didainius requested review from dataclouder and lvirbalas May 26, 2021 19:04

review changes:

3b05bd9

- corrected review notes - added point to changelog - update test suit and add test

Didainius reviewed May 28, 2021

View reviewed changes

govcd/sample_govcd_test_config.yaml Outdated Show resolved Hide resolved

remove storage profile from test config

da16b28

Didainius reviewed May 28, 2021

View reviewed changes

lelvisl requested a review from Didainius May 28, 2021 14:35

QueryAdminOrgVdcStorageProfileByID and QueryOrgVdcStorageProfileByID …

e917a0e

…now returns only one vdcStorageProfile

Didainius reviewed May 31, 2021

View reviewed changes

govcd/system.go Outdated Show resolved Hide resolved

change error handling for QueryOrgVdcStorageProfileByID and QueryAdmi…

a709b44

…nOrgVdcStorageProfileByID

Didainius approved these changes Jun 1, 2021

View reviewed changes

govcd/system.go Outdated Show resolved Hide resolved

govcd/system.go Outdated Show resolved Hide resolved

fixed typo

1864309

lvirbalas approved these changes Jun 2, 2021

View reviewed changes

dataclouder suggested changes Jun 11, 2021

View reviewed changes

govcd/system.go Show resolved Hide resolved

lelvisl added 2 commits June 12, 2021 14:20

Merge remote-tracking branch 'vmware/master' into AdminOrgVdcStorageP…

5e4a541

…rofileRecord # Conflicts: # CHANGELOG.md

add Test_QueryOrgVdcStorageProfileByID

5857089

updated fields for QueryResultAdminOrgVdcStorageProfileRecordType and QueryResultOrgVdcStorageProfileRecordType add skip/error in case of Client.IsSysAdmin - vmware#384

lelvisl requested a review from dataclouder June 12, 2021 12:21

Didainius mentioned this pull request Aug 18, 2022

Add AdminOrgVdcStorageProfile for querying (improved #375) #499

Merged

Didainius closed this Aug 18, 2022

lelvisl deleted the AdminOrgVdcStorageProfileRecord branch August 18, 2022 12:10

Didainius added a commit that referenced this pull request Aug 23, 2022

Add AdminOrgVdcStorageProfile for querying (improved #375) (#499)

bc692c3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AdminOrgVdcStorageProfile for querying #375

Add AdminOrgVdcStorageProfile for querying #375

lelvisl commented May 11, 2021

vmwclabot commented May 11, 2021

vmwclabot commented May 12, 2021

lelvisl commented May 14, 2021

lelvisl commented May 24, 2021

Didainius left a comment

lelvisl commented May 27, 2021

Didainius May 28, 2021

lelvisl May 28, 2021

lelvisl May 28, 2021

Didainius May 29, 2021

lelvisl May 31, 2021

Didainius commented May 28, 2021

Didainius left a comment

Didainius left a comment

lvirbalas left a comment

lelvisl commented Jun 10, 2021

dataclouder commented Jun 11, 2021

lelvisl commented Jun 12, 2021

lelvisl commented Sep 1, 2021

Didainius commented Aug 18, 2022

	func QueryAdminOrgVdcStorageProfileByID(vcdCli VCDClient, id string) ([]types.QueryResultAdminOrgVdcStorageProfileRecordType, error) {
	func QueryAdminOrgVdcStorageProfileByID(vcdCli VCDClient, id string) (types.QueryResultAdminOrgVdcStorageProfileRecordType, error) {

Add AdminOrgVdcStorageProfile for querying #375

Add AdminOrgVdcStorageProfile for querying #375

Conversation

lelvisl commented May 11, 2021

Issue: #373

Added:

vmwclabot commented May 11, 2021

vmwclabot commented May 12, 2021

lelvisl commented May 14, 2021

lelvisl commented May 24, 2021

Didainius left a comment

Choose a reason for hiding this comment

lelvisl commented May 27, 2021

Didainius May 28, 2021

Choose a reason for hiding this comment

lelvisl May 28, 2021

Choose a reason for hiding this comment

lelvisl May 28, 2021

Choose a reason for hiding this comment

Didainius May 29, 2021

Choose a reason for hiding this comment

lelvisl May 31, 2021

Choose a reason for hiding this comment

Didainius commented May 28, 2021

Didainius left a comment

Choose a reason for hiding this comment

Didainius left a comment

Choose a reason for hiding this comment

lvirbalas left a comment

Choose a reason for hiding this comment

lelvisl commented Jun 10, 2021

dataclouder commented Jun 11, 2021

lelvisl commented Jun 12, 2021

lelvisl commented Sep 1, 2021

Didainius commented Aug 18, 2022