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

Improve our Instance Metadata coverage in Azure. #49237

Merged
merged 1 commit into from Aug 3, 2017

Conversation

@brendandburns
Contributor

brendandburns commented Jul 19, 2017

No description provided.

@@ -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" {

This comment has been minimized.

@jingxu97

jingxu97 Jul 20, 2017

Contributor

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

@k8s-merge-robot k8s-merge-robot added size/L and removed size/M labels Jul 20, 2017

@colemickens

This comment has been minimized.

Contributor

colemickens commented Jul 20, 2017

/lgtm

but you're failing a go-lint test

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

This comment has been minimized.

@jackfrancis

jackfrancis Jul 26, 2017

Contributor

nit: comment describing exportable struct

This comment has been minimized.

@brendandburns

brendandburns Jul 26, 2017

Contributor

done.

baseURL string
}
func NewInstanceMetadata() *InstanceMetadata {

This comment has been minimized.

@jackfrancis

jackfrancis Jul 26, 2017

Contributor

ditto

This comment has been minimized.

@brendandburns

brendandburns Jul 26, 2017

Contributor

done.

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) {

This comment has been minimized.

@jackfrancis

jackfrancis Jul 26, 2017

Contributor

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

This comment has been minimized.

@brendandburns

brendandburns Jul 26, 2017

Contributor

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

This comment has been minimized.

@jackfrancis

jackfrancis Jul 26, 2017

Contributor

Sorry, just requesting a simple method comment.

// 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 {

This comment has been minimized.

@jackfrancis

jackfrancis Jul 26, 2017

Contributor

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

This comment has been minimized.

@jackfrancis

jackfrancis Jul 26, 2017

Contributor

or UnmarshalMetadataJSON

This comment has been minimized.

@brendandburns

brendandburns Jul 26, 2017

Contributor

done.

@jackfrancis

This comment has been minimized.

Contributor

jackfrancis commented Jul 26, 2017

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

@k8s-merge-robot k8s-merge-robot removed the lgtm label Jul 26, 2017

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Jul 26, 2017

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

@jackfrancis

This comment has been minimized.

Contributor

jackfrancis commented Jul 27, 2017

/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jul 27, 2017

@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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jul 27, 2017

[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:

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

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Jul 27, 2017

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

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Jul 27, 2017

/test pull-kubernetes-e2e-gce-etcd3

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Jul 27, 2017

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

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jul 31, 2017

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.


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

This comment has been minimized.

Member

jdumars commented Jul 31, 2017

/retest

@jdumars

This comment has been minimized.

Member

jdumars commented Jul 31, 2017

/sig azure

@jdumars

This comment has been minimized.

Member

jdumars commented Jul 31, 2017

@brendandburns can you take a look at this PR?

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Jul 31, 2017

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

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 1, 2017

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

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 1, 2017

/test pull-kubernetes-e2e-kops-aws

2 similar comments
@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 1, 2017

/test pull-kubernetes-e2e-kops-aws

@jdumars

This comment has been minimized.

Member

jdumars commented Aug 1, 2017

/test pull-kubernetes-e2e-kops-aws

@jdumars

This comment has been minimized.

Member

jdumars commented Aug 1, 2017

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

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 2, 2017

/test pull-kubernetes-e2e-kops-aws

@brendandburns brendandburns added the lgtm label Aug 2, 2017

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 2, 2017

self lgtm since this was only a rebase.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Aug 3, 2017

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

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Aug 3, 2017

@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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Aug 3, 2017

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

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

8 of 10 checks passed

pull-kubernetes-e2e-gce-etcd3 Jenkins job failed.
Details
Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce-etcd3
Details
cla/linuxfoundation brendandburns authorized
Details
pull-kubernetes-bazel Job succeeded.
Details
pull-kubernetes-e2e-kops-aws Jenkins job succeeded.
Details
pull-kubernetes-federation-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-node-e2e Jenkins job succeeded.
Details
pull-kubernetes-unit Jenkins job succeeded.
Details
pull-kubernetes-verify Jenkins job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment