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

Order node addresses based on cluster stack type #689

Merged
merged 1 commit into from
May 16, 2024

Conversation

krzykwas
Copy link
Contributor

/assign @aojea
/assign @marqc

@k8s-ci-robot
Copy link
Contributor

@krzykwas: GitHub didn't allow me to assign the following users: marqc.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @aojea
/assign @marqc

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 10, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If the repository mantainers determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 10, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @krzykwas. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 10, 2024
providers/gce/gce.go Outdated Show resolved Hide resolved
providers/gce/gce_instances.go Outdated Show resolved Hide resolved
providers/gce/gce.go Outdated Show resolved Hide resolved
providers/gce/gce_fake.go Show resolved Hide resolved
providers/gce/gce_instances.go Outdated Show resolved Hide resolved

// Both internal and external IPv6 addresses are written to this array
ipv6s, err := metadata.Get(fmt.Sprintf(networkInterfaceIPV6, nic))
if err != nil || ipv6s == "" {
klog.Infof("no internal IPV6 addresses found for node %v: %v", nodeName, err)
klog.Infof("No internal IPV6 addresses found for node %v: %v.", nodeName, err)
Copy link
Member

Choose a reason for hiding this comment

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

just an advice, you need to avoid modifying unrelated part of the code in the same commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are tiny readability-related enhancements. I can revert them, though, and submit in a different PR. Please let me know.

Copy link
Member

Choose a reason for hiding this comment

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

no, is not important, is for education, each commit has to be self contained, if you want to do unrelated changes you can use a different commit or a different PR, but if you make them in the same commit it is distracting for the reviewer and in case of reverts or bisect or cherry-pick it can have conflicts ...

@aojea
Copy link
Member

aojea commented May 15, 2024

/ok-to-test
/approve
/assign @marqc

overall LGTM but I couldn't review the test thoroughly so I leave the approval, but @marqc please do the final review, please pay attention to all the possible methods that can be called so we are sure we are consistently returning the same order

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 15, 2024
@k8s-ci-robot
Copy link
Contributor

@aojea: GitHub didn't allow me to assign the following users: marqc.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/ok-to-test
/approve
/assign @marqc

overall LGTM but I couldn't review the test thoroughly so I leave the approval, but @marqc please do the final review, please pay attention to all the possible methods that can be called so we are sure we are consistently returning the same order

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 15, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, krzykwas, marqc

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2024
@krzykwas
Copy link
Contributor Author

/retest

@krzykwas
Copy link
Contributor Author

@aojea offline @marqc confirmed he is ok with this PR.

@marqc
Copy link

marqc commented May 16, 2024

/lgtm

@k8s-ci-robot
Copy link
Contributor

@marqc: changing LGTM is restricted to collaborators

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-sigs/prow repository.

@aojea
Copy link
Member

aojea commented May 16, 2024

/lgtm

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2024
@k8s-ci-robot k8s-ci-robot merged commit 4e9dcf3 into kubernetes:master May 16, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants