Skip to content

Starting v2 release following API break#91

Merged
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
dims:move-to-klog-v2
Sep 19, 2019
Merged

Starting v2 release following API break#91
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
dims:move-to-klog-v2

Conversation

@dims
Copy link
Copy Markdown
Member

@dims dims commented Aug 27, 2019

In cd60aa4, we changed the API leading
to issues like:

src/k8s.io/client-go/transport/round_trippers.go:70:11: cannot convert klog.V(9) (type klog.Verbose) to type bool

So we should move to v2 of klog

Change-Id: Ibf270e0e6dd326fa91d18140e92139455cbe4de0

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:


@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 27, 2019
@dims dims requested review from munnerz and vincepri August 27, 2019 15:23
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 27, 2019
@dims dims requested a review from hoegaarden August 27, 2019 15:24
@dims
Copy link
Copy Markdown
Member Author

dims commented Aug 27, 2019

/hold

(for discussion)

@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 Aug 27, 2019
@dims
Copy link
Copy Markdown
Member Author

dims commented Aug 27, 2019

/assign @DirectXMan12

@dims
Copy link
Copy Markdown
Member Author

dims commented Aug 27, 2019

/assign @liggitt

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Aug 28, 2019

if we're committed to cd60aa4 (which still seems strange to me, see comment at #20 (comment)), then we should definitely rev the version to avoid breaking consumers.

in addition to this, I think we'd want to do something like create a release-1.x branch and v1.0.0 tag at the commit just prior to #20)

@DirectXMan12
Copy link
Copy Markdown

I replied a bit to your comment in #20 (comment) and #20 (comment).

TL;DR: this is a decent stopgap till we can get through a KEP to switch all of client-go and friends over to structured logging natively. That's a lot more work longterm, though (I'd like to get it done, but even if we were to start now, it'd still be a while).

@dims
Copy link
Copy Markdown
Member Author

dims commented Aug 29, 2019

+1 to get this in AND cut a release-1.x branch AND v1.0.0 tag at the commit just prior to #20

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Aug 30, 2019

I would recommend reverting #20, changing to v2, then considering reintroducing #20

@DirectXMan12
Copy link
Copy Markdown

we may want to do the /v2 branch approach as well, since a) it gives us a path to do bugfixes on v1 and b) considering the number of people trying to go get w/o module support.

@dims
Copy link
Copy Markdown
Member Author

dims commented Sep 19, 2019

/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 Sep 19, 2019
@liggitt
Copy link
Copy Markdown
Member

liggitt commented Sep 19, 2019

do you need to change go_import_path: k8s.io/klog/v2 in travis?

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Sep 19, 2019

and update the readme to note the minimum go version

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Sep 19, 2019

might need something like this in travis (dropping go 1.9):

...
go_import_path: k8s.io/klog/v2
...
go:
  - 1.10.x
  - 1.11.x
  - 1.12.x
  - 1.13.x
...

@dims dims force-pushed the move-to-klog-v2 branch 2 times, most recently from 7bd3f55 to 57f7ec6 Compare September 19, 2019 13:21
Comment thread .travis.yml
- 1.9.x
- 1.10.x
- 1.11.x
- 1.12.x
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add 1.13.x?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Sep 19, 2019

=== RUN   TestHeaderWithDir
--- FAIL: TestHeaderWithDir (0.00s)
    klog_test.go:217: log format error: 0 elements, error input does not match format:
        I0102 15:04:05.067890    1234 v2/klog_test.go:212] test
    klog_test.go:223: log format error: got:
        	"I0102 15:04:05.067890    1234 v2/klog_test.go:212] test\n"
        want:	"I0102 15:04:05.067890    1234 klog/klog_test.go:0] test\n"

In cd60aa4, we changed the API leading
to issues like:
```
src/k8s.io/client-go/transport/round_trippers.go:70:11: cannot convert klog.V(9) (type klog.Verbose) to type bool
```

So we should move to v2 of klog

Change-Id: Ibf270e0e6dd326fa91d18140e92139455cbe4de0
@dims
Copy link
Copy Markdown
Member Author

dims commented Sep 19, 2019

fixing test

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Sep 19, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2019
@k8s-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, liggitt

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

The pull request process is described here

Details 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 merged commit afedc4c into kubernetes:master Sep 19, 2019
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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants