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.

Sign up for GitHub

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

Jump to bottom

`azurerm_shared_image_gallery` - support for the sharing block #22221

Merged

katbyte merged 2 commits into hashicorp:main from myc2h6o:shared_gallery

Aug 3, 2023

Contributor

myc2h6o commented Jun 20, 2023 •

edited

To add support for Community Shared Gallery.
Added tests as well for VM to consume Community Shared Image Version:

terraform-provider-azurerm/internal/services/compute/linux_virtual_machine_resource.go

Line 302 in f70fe79

computeValidate.CommunityGalleryImageVersionID,

There will be another possible value for sharing_profile.permission to support Direct Shared Gallery after its GA so community_gallery_info is set to optional

github-actions bot added dependencies documentation size/XL labels

myc2h6o force-pushed the shared_gallery branch 3 times, most recently from 53252d3 to bf14b7b Compare

June 21, 2023 09:37

Contributor Author

myc2h6o commented Jun 21, 2023

Test result:

WodansSon requested changes

View reviewed changes

Collaborator

WodansSon left a comment

@myc2h6o thank you so much for this PR. This is looking really good so far, but I did leave a few comments. If you can take a look at those I will be happy to give this a second review to get this merged! 🚀

internal/services/compute/shared_image_gallery_resource.go Outdated

@@ @@ -53,6 +55,64 @@ func resourceSharedImageGallery() *pluginsdk.Resource { @@
               				Optional: true,
               			},
+              			"sharing_profile": {

Collaborator

WodansSon Jun 22, 2023

Suggested change

      
            			"sharing_profile": {
          
            			"sharing": {

Per the reference naming doc should we shorten this to just sharing?

internal/services/compute/shared_image_gallery_resource.go Outdated

+              							}, false),
+              						},
+              						"community_gallery_info": {

Collaborator

WodansSon Jun 22, 2023

Suggested change

      
            						"community_gallery_info": {
          
            						"community_gallery": {

Same here, as mentioned above, should we shorten this to just community_gallery?

internal/services/compute/shared_image_gallery_resource.go Outdated

+              										ForceNew:     true,
+              										ValidateFunc: validation.StringIsNotEmpty,
+              									},
+              									"public_name": {

Collaborator

WodansSon Jun 22, 2023

Suggested change

      
            									"public_name": {
          
            									"name": {

Should this be shortened to just name?

internal/services/compute/shared_image_gallery_resource.go Outdated

+              										ForceNew:     true,
+              										ValidateFunc: validation.StringIsNotEmpty,
+              									},
+              									"public_name_prefix": {

Collaborator

WodansSon Jun 22, 2023

Suggested change

      
            									"public_name_prefix": {
          
            									"prefix": {

Would it make more sense if this was just prefix or name_prefix?

internal/services/compute/shared_image_gallery_resource.go Outdated

		@@ -65,6 +125,7 @@ func resourceSharedImageGallery() *pluginsdk.Resource {

		func resourceSharedImageGalleryCreateUpdate(d *pluginsdk.ResourceData, meta interface{}) error {

Collaborator

WodansSon Jun 22, 2023

Since update is now supported I believe we should make this two separate functions (e.g. resourceSharedImageGalleryCreate and resourceSharedImageGalleryUpdate)

internal/services/compute/shared_image_gallery_resource.go Outdated

+              	if model := resp.Model; model != nil {
+              		if prop := model.Properties; prop != nil && prop.SharingProfile != nil && prop.SharingProfile.Permissions != nil {
+              			if *prop.SharingProfile.Permissions == galleries.GallerySharingPermissionTypesCommunity {

Collaborator

WodansSon Jun 22, 2023

Suggested change

      
            			if *prop.SharingProfile.Permissions == galleries.GallerySharingPermissionTypesCommunity {
          
            			if pointer.From(prop.SharingProfile.Permissions) == galleries.GallerySharingPermissionTypesCommunity {

To be consistent in the code should we migrate this to use the pointer helper?

NOTE: I am not going to mark all of the * code, but there are a lot of places in the code where this can be migrated as well. WDYT?

website/docs/r/shared_image_gallery.html.markdown Outdated


		* `community_gallery_info` - (Optional) A `community_gallery_info` block as defined below. Changing this forces a new resource to be created.

		~> NOTE `community_gallery_info` must be set when `permission` is set to `Community`.

Collaborator

WodansSon Jun 22, 2023

Suggested change

      
            ~> **NOTE** `community_gallery_info` must be set when `permission` is set to `Community`.
          
            ~> **NOTE:** `community_gallery_info` must be set when `permission` is set to `Community`.

Super minor, but should we add a colon : at the end of NOTE?

github-actions bot added the waiting-response label


          azurerm_shared_image_gallery - support for Community Gallery

dd3c05a

myc2h6o force-pushed the shared_gallery branch from bf14b7b to dd3c05a Compare

June 25, 2023 05:13

github-actions bot removed the waiting-response label

Contributor Author

myc2h6o commented Jun 25, 2023

Hi @WodansSon thanks for reviewing the change! I've updated the pr according to your comment, please take a look.

Contributor Author

myc2h6o commented Jun 25, 2023

Updated test result:

stephybun self-assigned this

stephybun added the service/images label

katbyte approved these changes

View reviewed changes

Collaborator

katbyte left a comment

LGTM 🎨

stephybun reviewed

View reviewed changes

website/docs/r/shared_image_gallery.html.markdown


		A `sharing` block supports the following:

		* `permission` - (Required) The permission of the Shared Image Gallery when sharing. The only possible value now is `Community`. Changing this forces a new resource to be created.

Member

stephybun Jun 26, 2023

The docs say this is in preview, so could we add a note explaining that:

Suggested change

      
            * `permission` - (Required) The permission of the Shared Image Gallery when sharing. The only possible value now is `Community`. Changing this forces a new resource to be created.
          
            * `permission` - (Required) The permission of the Shared Image Gallery when sharing. The only possible value now is `Community`. Changing this forces a new resource to be created.
          
            -> **Note:** This requires that the Preview Feature `Microsoft.Compute/CommunityGalleries` is enabled, see [the documentation](https://learn.microsoft.com/azure/virtual-machines/share-gallery-community?tabs=cli) for more information.

Contributor Author

myc2h6o Aug 2, 2023

thanks for pointing that out, I've updated the document. This shall be GAed already in most of the regions, including the regions used by our acc tests so test subscriptions shall be fine to run the new test

github-actions bot added the waiting-response label


          Update website/docs/r/shared_image_gallery.html.markdown

a910c12

Co-authored-by: stephybun <steph@hashicorp.com>

github-actions bot removed the waiting-response label

katbyte changed the title ~~azurerm_shared_image_gallery - support for Community Gallery~~ azurerm_shared_image_gallery - support for the sharing block

katbyte merged commit 4d1c9bf into hashicorp:main

23 checks passed

katbyte added a commit that referenced this pull request


          Update CHANGELOG.md #22221

0e3a4e9

github-actions bot added this to the v3.68.0 milestone

myc2h6o deleted the shared_gallery branch

August 3, 2023 02:01

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment