Improve our Instance Metadata coverage in Azure. #49237

brendandburns · 2017-07-19T21:10:48Z

No description provided.

jingxu97 · 2017-07-20T00:47:19Z

pkg/cloudprovider/providers/azure/azure_test.go

@@ -821,6 +821,13 @@ func TestSplitProviderID(t *testing.T) {
 	}
 }

+func TestMetadataURLGeneration(t *testing.T) {
+	fullPath := MakeMetadataURL("some/path")
+	if fullPath != "http://169.254.169.254/metadata/some/path" {


Can we use constant metaURL here to check the full path so that it will not break the test if the const is changed?

colemickens · 2017-07-20T16:26:51Z

/lgtm

but you're failing a go-lint test

jackfrancis · 2017-07-26T22:07:37Z

pkg/cloudprovider/providers/azure/azure_instance_metadata.go

@@ -61,28 +54,43 @@ type Subnet struct {
 	Prefix  string `json:"prefix"`
 }

+type InstanceMetadata struct {


nit: comment describing exportable struct

jackfrancis · 2017-07-26T22:08:00Z

pkg/cloudprovider/providers/azure/azure_instance_metadata.go

+	baseURL string
+}
+
+func NewInstanceMetadata() *InstanceMetadata {


jackfrancis · 2017-07-26T22:11:48Z

pkg/cloudprovider/providers/azure/azure_instance_metadata.go

 	if err != nil {
 		return "", err
 	}
 	return string(data), err
 }

-func queryMetadataBytes(path, format string) ([]byte, error) {
+func (i *InstanceMetadata) queryMetadataBytes(path, format string) ([]byte, error) {


One more comment nit, just to be consistent (cf: the // makeMetadataURL comment)

Not sure what you are asking for here... Can you clarify? Thanks.

Sorry, just requesting a simple method comment.

jackfrancis · 2017-07-26T22:36:48Z

pkg/cloudprovider/providers/azure/azure_instance_metadata.go

 // QueryMetadataJSON queries the metadata server and populates the passed in object
-func QueryMetadataJSON(path string, obj interface{}) error {
-	data, err := queryMetadataBytes(path, "json")
+func (i *InstanceMetadata) QueryMetadataJSON(path string, obj interface{}) error {


It'd be nice to incorporate into the name of this method the idea that it mutates the passed in object interface. QueryMetadataJSONUnmarshall? (ugh)

or UnmarshalMetadataJSON

jackfrancis · 2017-07-26T22:39:12Z

recommend that we withdraw PR #49081 and use this one instead

brendandburns · 2017-07-26T23:15:56Z

Comments mostly addressed, one that I'm not sure what the request is...

jackfrancis · 2017-07-27T05:10:14Z

/lgtm

k8s-ci-robot · 2017-07-27T05:10:21Z

@jackfrancis: changing LGTM is restricted to assignees, and only kubernetes org members may be assigned issues.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-github-robot · 2017-07-27T05:10:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: brendandburns, colemickens, jackfrancis

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cloudprovider/providers/azure/OWNERS~~ [brendandburns,colemickens]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

brendandburns · 2017-07-27T19:36:10Z

@jdumars @jackfrancis can you propose membership for yourself in the kubernetes org?

brendandburns · 2017-07-27T19:36:24Z

/test pull-kubernetes-e2e-gce-etcd3

brendandburns · 2017-07-27T19:36:41Z

(self lgtm/approve given lgtm from @jackfrancis above)

k8s-ci-robot · 2017-07-31T19:20:58Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jdumars · 2017-07-31T19:25:37Z

/retest

jdumars · 2017-07-31T19:27:29Z

/sig azure

jdumars · 2017-07-31T20:00:24Z

@brendandburns can you take a look at this PR?

brendandburns · 2017-07-31T23:58:03Z

@jdumars fixed, I don't know what happened there. git is still a foreign language for me sometimes :)

brendandburns · 2017-08-01T00:52:47Z

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-e2e-gce-etcd3

brendandburns · 2017-08-01T04:09:35Z

/test pull-kubernetes-e2e-kops-aws

brendandburns · 2017-08-01T04:49:51Z

/test pull-kubernetes-e2e-kops-aws

jdumars · 2017-08-01T17:52:06Z

/test pull-kubernetes-e2e-kops-aws

jdumars · 2017-08-01T20:08:08Z

/test pull-kubernetes-e2e-kops-aws
once more with feeling!

brendandburns · 2017-08-02T14:47:04Z

/test pull-kubernetes-e2e-kops-aws

brendandburns · 2017-08-02T14:47:25Z

self lgtm since this was only a rebase.

k8s-github-robot · 2017-08-03T01:06:35Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2017-08-03T01:59:28Z

@brendandburns: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-etcd3	`e03f02a`	link	`/test pull-kubernetes-e2e-gce-etcd3`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2017-08-03T02:11:09Z

Automatic merge from submit-queue (batch tested with PRs 49237, 49656, 49980, 49841, 49899)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 19, 2017

k8s-github-robot assigned jingxu97 and liggitt Jul 19, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jul 19, 2017

jingxu97 reviewed Jul 20, 2017

View reviewed changes

brendandburns force-pushed the imds2 branch from dbfb7a2 to f6fbf38 Compare July 20, 2017 04:34

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 20, 2017

k8s-ci-robot assigned colemickens Jul 20, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2017

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jul 20, 2017

jackfrancis reviewed Jul 26, 2017

View reviewed changes

brendandburns force-pushed the imds2 branch from f6fbf38 to 9d823b1 Compare July 26, 2017 23:14

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 26, 2017

brendandburns added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jul 27, 2017

brendandburns added this to the v1.8 milestone Jul 27, 2017

brendandburns removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jul 27, 2017

brendandburns added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 27, 2017

k8s-github-robot added kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 31, 2017

k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 31, 2017

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 31, 2017

k8s-ci-robot added the sig/azure label Jul 31, 2017

metadata improvements.

e03f02a

brendandburns force-pushed the imds2 branch from 2c50c35 to e03f02a Compare July 31, 2017 23:55

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 31, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 31, 2017

brendandburns added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2017

k8s-github-robot merged commit 82b95c0 into kubernetes:master Aug 3, 2017

Improve our Instance Metadata coverage in Azure. #49237

Improve our Instance Metadata coverage in Azure. #49237

Conversation

brendandburns commented Jul 19, 2017

Choose a reason for hiding this comment

colemickens commented Jul 20, 2017

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

jackfrancis commented Jul 26, 2017

brendandburns commented Jul 26, 2017

jackfrancis commented Jul 27, 2017

k8s-ci-robot commented Jul 27, 2017

k8s-github-robot commented Jul 27, 2017

brendandburns commented Jul 27, 2017

brendandburns commented Jul 27, 2017

brendandburns commented Jul 27, 2017

k8s-ci-robot commented Jul 31, 2017

jdumars commented Jul 31, 2017

jdumars commented Jul 31, 2017

jdumars commented Jul 31, 2017

brendandburns commented Jul 31, 2017

brendandburns commented Aug 1, 2017

brendandburns commented Aug 1, 2017

brendandburns commented Aug 1, 2017

jdumars commented Aug 1, 2017

jdumars commented Aug 1, 2017

brendandburns commented Aug 2, 2017

brendandburns commented Aug 2, 2017

k8s-github-robot commented Aug 3, 2017

k8s-ci-robot commented Aug 3, 2017

k8s-github-robot commented Aug 3, 2017