Skip to content
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

Add metadata support for Orgs and Disk resources/datasources #797

Merged
merged 23 commits into from
Mar 24, 2022

Conversation

adambarreiro
Copy link
Collaborator

Signed-off-by: abarreiro [email protected]

Depends on: vmware/go-vcloud-director#438

Description

This PR adds support for metadata maps in the vcd_org and vcd_independent_disk resources&datasources.

Extra

Refactored datasource_vcd_independent_disk, resource_vcd_independent_disk, datasource_vcd_org, resource_vcd_org to not use deprecated schema.Resource fields for create/update/delete/import, now it uses CreateContext, UpdateContext, ReadContext, DeleteContext.

Also in the above three items, functions return the diag.Diagnostics struct instead of plain Go errors, to adjust their signature to the new CreateContext, UpdateContext, ReadContext, DeleteContext fields.

abarreiro added 2 commits March 2, 2022 10:35
#
Signed-off-by: abarreiro <[email protected]>
@adambarreiro adambarreiro self-assigned this Mar 2, 2022
@adambarreiro adambarreiro marked this pull request as ready for review March 2, 2022 09:49
#
Signed-off-by: abarreiro <[email protected]>
@adambarreiro adambarreiro requested a review from mikeletux March 8, 2022 14:51
Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

Where are the documentation additions for the new fields?

Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

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

Just as @lvirbalas mentioned - I am missing documentation of these fields in docs.

Code looks and worked great, as I tested with tags org and disk as well as manually while observing statefile. I am good to approve in general, just a few minor comments in code and I have an ask for tests - could add a few additional steps (for future insurance):

  • Adding more than one key in the metadata block (I saw it is there for Org, but also could be for independent disk)
  • Where we have test with multiple steps, you could use the opportunity to set metadata in one step and remove it in the next (with additional check that it is not set after that). That way we could have coverage if we ever screw up metadata cleanup.

vcd/resource_vcd_independent_disk.go Outdated Show resolved Hide resolved
vcd/resource_vcd_independent_disk.go Outdated Show resolved Hide resolved
vcd/resource_vcd_org.go Outdated Show resolved Hide resolved
@adambarreiro
Copy link
Collaborator Author

Added documentation updates for disk and org resources, and org datasource. Disk datasource didn't need to be changed, as it only references the resource.

abarreiro added 2 commits March 11, 2022 14:01
fmt
Signed-off-by: abarreiro <[email protected]>
@adambarreiro
Copy link
Collaborator Author

Just as @lvirbalas mentioned - I am missing documentation of these fields in docs.

Code looks and worked great, as I tested with tags org and disk as well as manually while observing statefile. I am good to approve in general, just a few minor comments in code and I have an ask for tests - could add a few additional steps (for future insurance):

  • Adding more than one key in the metadata block (I saw it is there for Org, but also could be for independent disk)
  • Where we have test with multiple steps, you could use the opportunity to set metadata in one step and remove it in the next (with additional check that it is not set after that). That way we could have coverage if we ever screw up metadata cleanup.

Added a new step to update metadata for Disks and Orgs. For Orgs I need to check further as I'm getting an error, so PR can't be merged as it's now.

@Didainius
Copy link
Collaborator

Added a new step to update metadata for Disks and Orgs. For Orgs I need to check further as I'm getting an error, so PR can't be merged as it's now.

Ok. We can sync up on that or I'll be waiting for a heads up.

Signed-off-by: abarreiro <[email protected]>
@adambarreiro
Copy link
Collaborator Author

Added a new step to update metadata for Disks and Orgs. For Orgs I need to check further as I'm getting an error, so PR can't be merged as it's now.

Ok. We can sync up on that or I'll be waiting for a heads up.

Fixed in the last commit.

Signed-off-by: abarreiro <[email protected]>
@adambarreiro
Copy link
Collaborator Author

I'm requesting a new review @Didainius @vbauzysvmware, as I merged code from main with #789 which was a quite big change. So far, disks tests pass, so it should be OK, but just in case you have new suggestions or things to improve.

Signed-off-by: abarreiro <[email protected]>
vcd/datasource_vcd_independent_disk_test.go Outdated Show resolved Hide resolved
vcd/datasource_vcd_org_test.go Show resolved Hide resolved
vcd/resource_vcd_independent_disk.go Outdated Show resolved Hide resolved
vcd/resource_vcd_org_test.go Outdated Show resolved Hide resolved
website/docs/r/independent_disk.html.markdown Outdated Show resolved Hide resolved
website/docs/r/independent_disk.html.markdown Outdated Show resolved Hide resolved
abarreiro added 2 commits March 22, 2022 15:22
website/docs/r/independent_disk.html.markdown Outdated Show resolved Hide resolved
website/docs/r/org.html.markdown Outdated Show resolved Hide resolved
abarreiro added 6 commits March 23, 2022 12:53
Signed-off-by: abarreiro <[email protected]>
fmt
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

now LGTM. Thank you

@adambarreiro adambarreiro removed the request for review from dataclouder March 23, 2022 14:22
Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

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

LGTM - thanks

Copy link
Contributor

@mikeletux mikeletux left a comment

Choose a reason for hiding this comment

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

LGTM! Approved!

@vmwclabot
Copy link
Member

@adambarreiro, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

1 similar comment
@vmwclabot
Copy link
Member

@adambarreiro, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

abarreiro added 2 commits March 24, 2022 12:44
Signed-off-by: abarreiro <[email protected]>
@adambarreiro adambarreiro force-pushed the add-metadata-org-disk-vapptempl branch from ce9d788 to 5c5911e Compare March 24, 2022 11:49
@adambarreiro
Copy link
Collaborator Author

Test pass, ready to merge once all conversations are solved.

@adambarreiro adambarreiro merged commit 79cb534 into vmware:main Mar 24, 2022
@adambarreiro adambarreiro deleted the add-metadata-org-disk-vapptempl branch March 24, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants