New Data Source: `azurerm_function_app_host_keys` #7902

3mard · 2020-07-26T11:54:59Z

This PR closes #699

3mard · 2020-07-26T11:56:20Z

azurerm/internal/services/web/data_source_function_app_host_keys.go

+				},
+			},
+
+			"system_keys": {


I am not sure about system_keys .. it exists in the api, however I never used it nor was able to see it being set while I was testing

If its value is not returned by the API / we're not using it, then it should be removed.

I agree with @jackofallops, I am going to remove it

georghildebrand · 2020-08-13T12:42:32Z

@njuCZ , do you know when this feature will be merged?

njuCZ · 2020-08-14T08:41:42Z

@georghildebrand I am sorry that I don't know the detail plan. If you are urgent about this PR, I could make a ping to the reviewers, or you could directly ping the reviewers in the slack channel.

georghildebrand · 2020-08-17T07:55:07Z

@njuCZ , thank you for having a look. Its quite urgent for me. I will also ping the devs in the slack channel

georghildebrand · 2020-08-17T08:28:17Z

@njuCZ , just realized that I have no slack account anymore as i switched jobs. Could you ping the engineers please?

3mard · 2020-08-17T08:35:44Z

@georghildebrand you can use this link to request an invitation : https://join.slack.com/t/terraform-azure/shared_invite/enQtNDMzNjQ5NzcxMDc3LWNiY2ZhNThhNDgzNmY0MTM0N2MwZjE4ZGU0MjcxYjUyMzRmN2E5NjZhZmQ0ZTA1OTExMGNjYzA4ZDkwZDYxNDE

sgettys · 2020-08-18T20:45:01Z

I am also waiting on this!

georghildebrand · 2020-08-19T08:23:51Z

@sgettys , please consider upvoting in slack. actually there is currently now response from the devs. https://app.slack.com/client/TB9RVKMGW/CJD3VK9Q9/thread/CJD3VK9Q9-1596945807.467000

magodo

@3mard Thanks for this PR!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍

magodo · 2020-08-24T06:49:06Z

azurerm/internal/services/web/data_source_function_app_host_keys.go

+		Schema: map[string]*schema.Schema{
+			"name": {
+				Type:     schema.TypeString,
+				ForceNew: true,


ForceNew is not needed for a data source:

Suggested change

ForceNew: true,

azurerm/internal/services/web/data_source_function_app_host_keys.go

+				},
+			},
+
+			"system_keys": {


magodo · 2020-08-24T07:24:42Z

azurerm/internal/services/web/data_source_function_app_host_keys.go

+	if err := d.Set("master_key", res.MasterKey); err != nil {
+		return err
+	}
+	if err = d.Set("function_keys", res.FunctionKeys); err != nil {


Can we use utils.FlattenMapStringPtrString() here?

Suggested change

if err = d.Set("function_keys", res.FunctionKeys); err != nil {

if err = d.Set("function_keys", utils.FlattenMapStringPtrString(res.FunctionKeys)); err != nil {

actually instead can we expose the values from this field in a defensive manner (see above) - exposing a map of values isn't a great UX since these can change in the API response

I'd like to go with @tombuildsstuff approach on this if that's okay with @magodo

@3mard Sure, that makes more sense

magodo · 2020-08-24T07:24:58Z

azurerm/internal/services/web/data_source_function_app_host_keys.go

+	if err = d.Set("function_keys", res.FunctionKeys); err != nil {
+		return err
+	}
+	if err = d.Set("system_keys", res.SystemKeys); err != nil {


Same as above:

Suggested change

if err = d.Set("system_keys", res.SystemKeys); err != nil {

if err = d.Set("system_keys", utils.FlattenMapStringPtrString(res.SystemKeys)); err != nil {

magodo · 2020-08-24T07:25:51Z

azurerm/internal/services/web/data_source_function_app_host_keys.go

+	}
+
+	d.SetId(time.Now().UTC().String())
+	if err := d.Set("master_key", res.MasterKey); err != nil {


For primitive type, there is no need to error check for d.Set:

Suggested change

if err := d.Set("master_key", res.MasterKey); err != nil {

d.Set("master_key", res.MasterKey)

azurerm/internal/services/web/registration.go

tombuildsstuff

hey @3mard

Thanks for this PR :)

I've taken a look through and left some comments inline but if we can fix those up then this otherwise LGTM 👍

Thanks!

tombuildsstuff · 2020-08-24T16:37:35Z

azurerm/internal/services/web/data_source_function_app_host_keys.go

+					Type: schema.TypeString,
+				},
+				Sensitive: true,
+			},


rather than exposing this as a Map - can we lookup the data in a defensive fashion and output those specific fields, so that users don't have to deal with breaking changes in the response format here e.g.

defaultFunctionKey := "" if v, ok := keys["default"]; ok { defaultFunctionKey = v } d.Set("default_function_key", defaultFunctionKey)

tombuildsstuff · 2020-08-24T16:39:00Z

azurerm/internal/services/web/data_source_function_app_host_keys.go

+		return fmt.Errorf("Error making Read request on AzureRM Function App Hostkeys %q: %+v", name, err)
+	}
+
+	d.SetId(time.Now().UTC().String())


can we use the ID of the Function App/Key returned here, rather than a date which'll change all the time (and causes issues in Terraform 0.13+)

@tombuildsstuff I don't think I have the access to the function Id in the response AFAIK,

should I do a
client.Get(ctx, resourceGroup, name)
just to get the function Id ?

makes sense for now - we'll switch this out for something else later on 👍

Oh sorry for the confusion @tombuildsstuff , I went with your suggested solution, (one more API request to retrieve the function Id ), should I keep it like this?

tombuildsstuff · 2020-08-24T16:39:26Z

azurerm/internal/services/web/data_source_function_app_host_keys.go

+	if err := d.Set("master_key", res.MasterKey); err != nil {
+		return err
+	}
+	if err = d.Set("function_keys", res.FunctionKeys); err != nil {


actually instead can we expose the values from this field in a defensive manner (see above) - exposing a map of values isn't a great UX since these can change in the API response

tombuildsstuff · 2020-08-24T16:40:05Z

website/docs/d/function_app_host_keys.html.markdown

+layout: 'azurerm'
+page_title: 'Azure Resource Manager: azurerm_function_app_host_keys'
+description: |-
+  Gets HostKeys of an existing Function App.


Suggested change

Gets HostKeys of an existing Function App.

Gets the Host Keys of an existing Function App.

tombuildsstuff · 2020-08-24T16:40:17Z

website/docs/d/function_app_host_keys.html.markdown

+
+# Data Source: azurerm_function_app_host_keys
+
+Use this data source to fetch the Hostkeys of an existing Function App


Suggested change

Use this data source to fetch the Hostkeys of an existing Function App

Use this data source to fetch the Host Keys of an existing Function App

tombuildsstuff · 2020-08-24T16:40:21Z

website/docs/d/function_app_host_keys.html.markdown

+
+```hcl
+data "azurerm_function_app_host_keys" "example" {
+  name                = "test-azure-functions"


Suggested change

name = "test-azure-functions"

name = "example-function"

website/docs/d/function_app_host_keys.html.markdown

tombuildsstuff · 2020-08-24T16:41:04Z

website/docs/d/function_app_host_keys.html.markdown

+
+- `function_keys` - A key-value pair of Host level function keys.
+
+- `system_keys` - A key-value pair of system keys


(as above) can we make these specific fields rather than maps

Co-authored-by: Tom Harvey <[email protected]>

…3mard/terraform-provider-azurerm into add-data-source-function-appp-host-keys

magodo

@3mard

I've taken another look through and left some comments inline, one thing I'm uncertain is about how should we set the ID for this data source, maybe Tom shall shed some light on it.

Otherwise, this is mostly looking good to me 👍

azurerm/internal/services/web/data_source_function_app_host_keys.go

website/docs/d/function_app_host_keys.html.markdown

…ys.go Co-authored-by: magodo <[email protected]>

…vider-azurerm into add-data-source-function-appp-host-keys

magodo

@3mard

It almost look good to me, except a minor wording change. Otherwise, it should be good to go 🚀

website/docs/d/function_app_host_keys.html.markdown

Co-authored-by: magodo <[email protected]>

magodo · 2020-09-08T06:44:19Z

update CHANGELOG.md for #7902

ghost · 2020-09-10T14:27:07Z

This has been released in version 2.27.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.27.0"
}
# ... other configuration ...

ghost · 2020-10-08T14:50:19Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

3mard added 3 commits July 26, 2020 01:52

implement data_source_function_app_host_keys

e934ff3

Add Docs

32ed702

add tests

0ac23dd

ghost added size/L documentation labels Jul 26, 2020

3mard commented Jul 26, 2020

View reviewed changes

3mard mentioned this pull request Jul 26, 2020

Feature request: export Functions host keys #699

Closed

3mard added 2 commits July 26, 2020 14:49

lint

9571c37

mark master_key, function_keys and system_keys as Sensitive

89bb382

njuCZ mentioned this pull request Aug 5, 2020

Support for Getting Function App host keys #8008

Closed

magodo requested changes Aug 24, 2020

View reviewed changes

tombuildsstuff added new-data-source service/functions Function Apps labels Aug 24, 2020

tombuildsstuff suggested changes Aug 24, 2020

View reviewed changes

tombuildsstuff added the waiting-response label Aug 24, 2020

3mard and others added 3 commits August 29, 2020 23:15

Update website/docs/d/function_app_host_keys.html.markdown

6c27b2c

Co-authored-by: Tom Harvey <[email protected]>

fix feedback

8aefe7e

Merge branch 'add-data-source-function-appp-host-keys' of github.com:…

1bae371

…3mard/terraform-provider-azurerm into add-data-source-function-appp-host-keys

3mard force-pushed the add-data-source-function-appp-host-keys branch from 6b2ba8d to 1bae371 Compare August 31, 2020 22:24

3mard requested review from magodo, jackofallops and tombuildsstuff September 1, 2020 08:35

magodo requested changes Sep 2, 2020

View reviewed changes

azurerm/internal/services/web/data_source_function_app_host_keys.go Outdated Show resolved Hide resolved

azurerm/internal/services/web/data_source_function_app_host_keys.go Show resolved Hide resolved

website/docs/d/function_app_host_keys.html.markdown Outdated Show resolved Hide resolved

3mard and others added 2 commits September 2, 2020 09:53

Update azurerm/internal/services/web/data_source_function_app_host_ke…

41830d2

…ys.go Co-authored-by: magodo <[email protected]>

update docs

fe3b3d9

3mard requested a review from magodo September 2, 2020 08:18

3mard added 2 commits September 2, 2020 10:54

Merge branch 'master' of github.com:terraform-providers/terraform-pro…

e70f9e9

…vider-azurerm into add-data-source-function-appp-host-keys

fix lint

5db545c

jackofallops assigned magodo Sep 4, 2020

jackofallops added this to the v2.27.0 milestone Sep 4, 2020

magodo requested changes Sep 4, 2020

View reviewed changes

website/docs/d/function_app_host_keys.html.markdown Outdated Show resolved Hide resolved

Update website/docs/d/function_app_host_keys.html.markdown

22489cb

Co-authored-by: magodo <[email protected]>

3mard requested a review from magodo September 4, 2020 09:04

magodo changed the title ~~Add new data source function_app_host_keys~~ New Data Source: function_app_host_keys Sep 8, 2020

magodo changed the title ~~New Data Source: function_app_host_keys~~ New Data Source: azurerm_function_app_host_keys Sep 8, 2020

magodo merged commit 153cf39 into hashicorp:master Sep 8, 2020

magodo added a commit that referenced this pull request Sep 8, 2020

update CHANGELOG.md for #7902

3464dd3

update CHANGELOG.md for #7902

sschmeck mentioned this pull request Sep 8, 2020

Extend azurerm_function_app_host_keys with Event Grid system key #8385

Closed

ghost locked as resolved and limited conversation to collaborators Oct 8, 2020

	if err = d.Set("function_keys", res.FunctionKeys); err != nil {
	if err = d.Set("function_keys", utils.FlattenMapStringPtrString(res.FunctionKeys)); err != nil {

	if err = d.Set("system_keys", res.SystemKeys); err != nil {
	if err = d.Set("system_keys", utils.FlattenMapStringPtrString(res.SystemKeys)); err != nil {

	if err := d.Set("master_key", res.MasterKey); err != nil {
	d.Set("master_key", res.MasterKey)

	Gets HostKeys of an existing Function App.
	Gets the Host Keys of an existing Function App.


		# Data Source: azurerm_function_app_host_keys

		Use this data source to fetch the Hostkeys of an existing Function App

	Use this data source to fetch the Hostkeys of an existing Function App
	Use this data source to fetch the Host Keys of an existing Function App


		- `function_keys` - A key-value pair of Host level function keys.

		- `system_keys` - A key-value pair of system keys

New Data Source: azurerm_function_app_host_keys #7902

New Data Source: azurerm_function_app_host_keys #7902

Conversation

3mard commented Jul 26, 2020

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georghildebrand commented Aug 13, 2020 • edited Loading

njuCZ commented Aug 14, 2020

georghildebrand commented Aug 17, 2020

georghildebrand commented Aug 17, 2020

3mard commented Aug 17, 2020 • edited Loading

sgettys commented Aug 18, 2020

georghildebrand commented Aug 19, 2020

magodo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magodo left a comment

Choose a reason for hiding this comment

magodo left a comment • edited Loading

Choose a reason for hiding this comment

magodo commented Sep 8, 2020

ghost commented Sep 10, 2020

ghost commented Oct 8, 2020

New Data Source: `azurerm_function_app_host_keys` #7902

New Data Source: `azurerm_function_app_host_keys` #7902

georghildebrand commented Aug 13, 2020 •

edited

Loading

3mard commented Aug 17, 2020 •

edited

Loading

magodo left a comment •

edited

Loading

magodo left a comment •

edited

Loading