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.

Sign up for GitHub

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

Jump to bottom

✨ Update klog dependency to v2 #779

Merged
merged 2 commits into from
Mar 16, 2021
Merged

✨ Update klog dependency to v2 #779

merged 2 commits into from
Mar 16, 2021

Conversation

stmcginnis
Copy link
Contributor

What this PR does / why we need it:

The klog v1 dependency used is quite old now, and the k/k repo has
switched to more recent v2 versions for quite some time now.

This updates klog usage to use the v2 package version.

Special notes for your reviewer:

This is part of an overall effort along with updating the k-sigs/cluster-api repo here:

kubernetes-sigs/cluster-api#4284

Release note:

NONE

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 11, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @stmcginnis. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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/test-infra repository.

@k8s-ci-robot
Copy link
Contributor

Welcome @stmcginnis!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-openstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-openstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 11, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 11, 2021

Build failed.

@sbueringer
Copy link
Member

/ok-to-test

/approve

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 11, 2021
@sbueringer
Copy link
Member

recheck

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 11, 2021

Build failed.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 11, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 11, 2021

Build failed.

@sbueringer
Copy link
Member

/approve cancel
Might have been a bit to optimistic :)

@stmcginnis Can you rebase onto current master. I'm not sure why but Openlab seems to build with a pretty old configuration (Kubernetes 1.18 test with go 1.13)
OpenLab fails with an error I also had on another PR before upgrading to 1.16 in OpenLab.

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2021
@stmcginnis
Copy link
Contributor Author

This is current with master, so not sure what's up with OpenLab. Has that been stable lately? It looks like a config issue there.

Something definitely is not right though. The .zuul.yaml file locally doesn't even list cluster-api-provider-openstack-current-acceptance-test-v1.18. Though I suppose zuul could be picking that up from elsewhere.

@sbueringer
Copy link
Member

Okay. Yeah pretty strange. I have a pr open since today to drop openlab/zuul in favor of prow/gcp/devstack

@stmcginnis
Copy link
Contributor Author

I think that's a very good plan. I used to be involved in OpenLab. It hasn't taken off like originally planned. ;)

@sbueringer
Copy link
Member

Yeah unfortunately. It is stable but only when we hit a cluster with the right CPUs. That's why we run 3 tests at the same time since my pr today and fail instantly when we detect the wrong CPUs.

@stmcginnis
Copy link
Contributor Author

There should be some way to configure things on the OpenLab side to actually make sure the right kinds of nodes are scheduled. But unfortunately I can't recall how all that is done.

@sbueringer
Copy link
Member

There should be some way to configure things on the OpenLab side to actually make sure the right kinds of nodes are scheduled. But unfortunately I can't recall how all that is done.

I have the right nodes (sizes?) but was trying to get it configured that we avoid certain clusters/cpus but without success. But as we were using devstack there anyway (because I couldnt get a real octavia) gcp should be the better choice.

@jichenjc
Copy link
Contributor

recheck

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 12, 2021

Build failed.

@hidekazuna
Copy link
Contributor

@stmcginnis I wonder why this PR does not have go.sum. Did you run make modules?

@stmcginnis
Copy link
Contributor Author

Did you run make modules?

Yep, I ran it initially on the first commit. I added the second commit when I realized I originally ran it with go 1.15, but there were additional changes once I ran it with 1.16.

@hidekazuna
Copy link
Contributor

Did you run make modules?

Yep, I ran it initially on the first commit. I added the second commit when I realized I originally ran it with go 1.15, but there were additional changes once I ran it with 1.16.

Thanks, understand.

@hidekazuna
Copy link
Contributor

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2021
@jichenjc
Copy link
Contributor

recheck

@jichenjc
Copy link
Contributor

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 12, 2021

Build failed.

@sbueringer
Copy link
Member

@jichenjc please let's wait a bit until either: #780 or #759 is merged and we have green tests here. I think the problem here is OpenLab and not the change, but I really would like to not merge with a failed conformance test.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2021
@chrischdi chrischdi mentioned this pull request Mar 12, 2021
Merged
@jichenjc
Copy link
Contributor

@sbueringer thanks for the reminder, I agree we need wait for green test first

@prankul88
Copy link
Contributor

Yep, I ran it initially on the first commit. I added the second commit when I realized I originally ran it with go 1.15, but there were additional changes once I ran it with 1.16.

@stmcginnis I tried updating to klog v2 in my local with go1.16.2 and ran make modules, which brought changes to go.sum file as well. Wanted to understand this.

@stmcginnis
Copy link
Contributor Author

@prankul88 - that's odd - and a little concerning. I'm running go 1.16.1, but I would hope there wouldn't be any changes due to that.

I ran make modules again to see if maybe there were changes in the dependencies themselves that would cause those differences, but it comes back clean for me. So it does indeed seem to be the difference in bugfix version of go itself.

@stmcginnis
Copy link
Contributor Author

I upgraded locally to go 1.16.2 and ran make modules again, but it comes back clean for me. So not really sure what's going on there.

@prankul88
Copy link
Contributor

@stmcginnis Not sure either. Will double check again in my system.

@sbueringer
Copy link
Member

sbueringer commented Mar 15, 2021 •
edited
Loading

/test pull-cluster-api-provider-openstack-make-conformance
(prow test has been merged)

@stmcginnis
Update klog dependency to v2
f768acf 
The klog v1 dependency used is quite old now, and the k/k repo has
switched to more recent v2 versions for quite some time now.

This updates klog usage to use the v2 package version.

Signed-off-by: Sean McGinnis <[email protected]>
@sbueringer
Copy link
Member

@stmcginnis looks like Prow is happy, at least :)

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hidekazuna, sbueringer, stmcginnis

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:
  • OWNERS [hidekazuna,sbueringer]

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

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 15, 2021

Build failed.

@sbueringer
Copy link
Member

sbueringer commented Mar 15, 2021 •
edited
Loading

Let's merge it. The code already had enough reviews and now the build is also green.

I assume the issues @prankul88 is seeing are unrelated, but feel free to open an issue for those, maybe we find out what the problem is.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2021
@sbueringer
Copy link
Member

/test pull-cluster-api-provider-openstack-make-conformance

@jichenjc
Copy link
Contributor

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2021
@k8s-ci-robot k8s-ci-robot merged commit 3530a84 into kubernetes-sigs:master Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hidekazuna hidekazuna Awaiting requested review from hidekazuna

@jichenjc jichenjc Awaiting requested review from jichenjc

Assignees

@sbueringer sbueringer

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@stmcginnis @k8s-ci-robot @sbueringer @jichenjc @hidekazuna @prankul88