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

azure: support for marketplace plan information #5970

Merged
merged 2 commits into from
Mar 9, 2018

Conversation

boumenot
Copy link
Collaborator

@boumenot boumenot commented Mar 5, 2018

Yet another attempt at supporting Marketplace images (aka plan info). This PR differs from the previous attempt in that it explicitly provides settings for plan info whereas the previous attempt allowed arbitrary patching.

The following settings are required to use plan info. There's an optional fourth option called plan_promotion_code.

  "plan_name": "my-plan-name",
  "plan_product": "my-plan-product",
  "plan_publisher": "my-plan-publisher",

Closes #3506

@boumenot boumenot requested a review from a team as a code owner March 5, 2018 09:55
@boumenot boumenot force-pushed the pr-azure-plan-info branch from 3043767 to d2e593d Compare March 5, 2018 17:05
@SwampDragons
Copy link
Contributor

Idle/bikeshed thought -- I wonder if, from a UX perspective, it makes more sense to have these options bundled into a map like with source_ami_filter, since they're related so closely.

@boumenot
Copy link
Collaborator Author

boumenot commented Mar 6, 2018

Good question. Let me describe what this represents, and you can tell me if it maps well. I think the source filter is more about image selection process, whereas this is more for billing.

Azure has platform images for things like Windows, Linux, FreeBSD, etc. There are also Marketplace images, which are images that have a slight different rate plan (way customers are charged). Marketplace images have to be called out specially with these additional plan metadata. It isn't part of a selection process, but rather a billing calculation.

@SwampDragons
Copy link
Contributor

I think it's ultimately fine either way. My instinct was just that it feels like these four items represent one feature rather than several features, so it might help users to have them explicitly combined in that way, under, for example, a plan_information map, since that's a term you use within azure (I think?) to lump them together already. If you don't think it's worth changing, I'll merge as is.

@boumenot
Copy link
Collaborator Author

boumenot commented Mar 6, 2018

Explain more. I think I misunderstood your first request, and I like the idea of making them logically combined. Do you have a code example that I can refer too?

@SwampDragons
Copy link
Contributor

The mapstructure module is pretty smart, so you can just change your config to something like this

type PlanInformation struct {
	PlanName          string `mapstructure:"plan_name"`
	PlanProduct       string `mapstructure:"plan_product"`
	PlanPublisher     string `mapstructure:"plan_publisher"`
	PlanPromotionCode string `mapstructure:"plan_promotion_code"`
}

type Config struct {
	common.PackerConfig `mapstructure:",squash"`
	[...]
	// Plan Info
	PlanInfo PlanInformation `mapstructure:"plan_info"`

which allows you to read a map of strings from the packer config, e.g.

            [...]
            "plan_info": {
                "plan_name": "myplan",
                "plan_product": "myproduct",
                "plan_publisher": "mypublisher",
                "plan_promotion_code": "mycode"
            },

And then you can access that information through normal struct access methods. So your validation for example would be

	if c.PlanInfo.PlanName != "" || c.PlanInfo.PlanProduct != "" || c.PlanInfo.PlanPublisher != "" || c.PlanInfo.PlanPromotionCode != "" {
		if c.PlanInfo.PlanName == "" || c.PlanInfo.PlanProduct == "" || c.PlanInfo.PlanPublisher == "" {
			errs = packer.MultiErrorAppend(errs, fmt.Errorf("if either plan_name, plan_product, plan_publisher, or plan_promotion_code are defined then plan_name, plan_product, and plan_publisher must be defined"))
		}
	}

My gut just says doing it this way would help the user and people reading the docs understand explicitly that those items are only used as part of the same feature, and are tied to one another. But like I said, this is more of an idle thought / bikeshed and I'm happy to keep it your way.

@boumenot
Copy link
Collaborator Author

boumenot commented Mar 6, 2018

Boom! That's great! I wish I had done this for some of the other things I had implemented. I will make this change, and update the code and docs.

Thank you for the review!

@SwampDragons
Copy link
Contributor

🎉 glad it was helpful!

@cloudbooster
Copy link
Contributor

Thanks @boumenot this is great. We need to have the plan information that you are taking added to Tags for the generated Images . Tags will look like

Key Value
PlanName User provided value
PlanProduct User provided value
PlanPublisher User provided value
PlanPromotionCode User provided value

Enable access to image : https://aka.ms/azuremarketplaceapideployment

I would also like to point out that not all Azure marketplace images would work for programatic deployments as publishers control that, so we may have failures. I am happy to do integration testing, just give me a private.

@boumenot
Copy link
Collaborator Author

boumenot commented Mar 8, 2018

There is a limitation on the number of tags a user can set. If there are too many tags defined as a result of the user setting them, and this feature setting them I am going to just error out with an appropriate message.

I posted a private build for Linux, Mac, and Windows in #3506 . This build does not have any of the changes recommended in this thread, but I will add them.

@boumenot boumenot force-pushed the pr-azure-plan-info branch from 5d1f147 to 1ef491d Compare March 9, 2018 06:43
@SwampDragons
Copy link
Contributor

@boumenot ping me when you're ready for final review/merge :)

@boumenot
Copy link
Collaborator Author

boumenot commented Mar 9, 2018

@SwampDragons - I'm ready.

Copy link
Contributor

@cloudbooster cloudbooster left a comment

Choose a reason for hiding this comment

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

Looks good .. thanks Chirs .. I will test today with the binary that you provided

@@ -162,6 +162,17 @@ Providing `temp_resource_group_name` or `location` in combination with `build_re
`Linux` this configures an SSH authorized key. For `Windows` this
configures a WinRM certificate.

- `plan_name` (string) The plan name. This setting (`plan_product`, `plan_publisher`, and `plan_promotion_code`) are
only needed for Marketplace images. Please refer to [Deploy an image with Marketplace terms](https://docs.microsoft.com/en-us/azure/virtual-machines/linux/cli-ps-findimage#deploy-an-image-with-marketplace-terms) for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also put that if the above step is not done then the image creation will fail. Also using of the built image will also require the same step for the subscription where the images will get used.

Copy link
Collaborator

@paulmey paulmey left a comment

Choose a reason for hiding this comment

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

LGTM

@mwhooker mwhooker added this to the v1.2.2 milestone Mar 9, 2018
@SwampDragons SwampDragons merged commit b1eaaed into hashicorp:master Mar 9, 2018
@boumenot boumenot deleted the pr-azure-plan-info branch August 1, 2018 06:37
@ghost
Copy link

ghost commented Mar 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating image from some Azure Marketplace Images requires additional items in the ARM template.
5 participants