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 to cert-manager 1.7 #19

Merged
merged 5 commits into from
Mar 29, 2022
Merged

Update to cert-manager 1.7 #19

merged 5 commits into from
Mar 29, 2022

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Jan 25, 2022 •
edited
Loading

  • Updated to cert-manager 1.7.1
  • Update to Go 1.17 to work around some go mod problems with transitive dependencies
  • Adjusted code that uses logr which now uses structs instead of interfaces.

wallrj added 3 commits January 25, 2022 18:00
@wallrj
Depend on cert-manager 1.7
c1bd330 
Signed-off-by: Richard Wall <[email protected]>
@wallrj
go mod tidy
a21f27d 
Signed-off-by: Richard Wall <[email protected]>
@wallrj
Refactor to use new logr.Logger struct
277e4a6 
Signed-off-by: Richard Wall <[email protected]>
@jetstack-bot jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 25, 2022
@jetstack-bot jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 25, 2022
@wallrj
Update to Go 1.17
0df2ab9 
Signed-off-by: Richard Wall <[email protected]>
@jetstack-bot jetstack-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 25, 2022
wallrj
wallrj commented Jan 25, 2022
View reviewed changes
Copy link
Member Author

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

I'll need to update the testing repo to run these tests with Go 1.17

example/main.go Show resolved Hide resolved
go.mod Show resolved Hide resolved
test/log/test_logr.go
@@ -1,66 +0,0 @@
/*
Copy link
Member Author

Choose a reason for hiding this comment

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

There's a TestLogger in the go-logr package so I used that and removed this.

munnerz
munnerz reviewed Jan 26, 2022
View reviewed changes
go.mod Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
test/util/testutil.go
@@ -118,7 +119,7 @@ func RunTestDriver(t *testing.T, opts DriverOptions) (DriverOptions, csi.NodeCli
WriteKeypair: opts.WriteKeypair,
})

d := driver.NewWithListener(lis, opts.Log, driver.Options{
d := driver.NewWithListener(lis, *opts.Log, driver.Options{
Copy link
Member

Choose a reason for hiding this comment

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

Hm, previously it would be possible for a user to pass a nil pointer here. Now it isn't (and a user would need to create a Discard logger). Would it make sense to take a pointer here instead, and if it's nil, instantiate a discard logger automatically?

Copy link
Member

Choose a reason for hiding this comment

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

(I don't mind either way on this ^ just conscious of the change in method signature/how the caller calls this)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's already handled above, right? We create a TestLogger if opts.Log is nil.
I will leave it like this for now and you can improve it in a followup PR if necessary.

@jetstack-bot jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2022
@wallrj wallrj mentioned this pull request Jan 27, 2022
Merged
munnerz
munnerz reviewed Feb 2, 2022
View reviewed changes
manager/manager.go
@@ -150,7 +150,7 @@ func NewManager(opts Options) (*Manager, error) {
lister: lister,
metadataReader: opts.MetadataReader,
clock: opts.Clock,
log: opts.Log,
log: *opts.Log,
Copy link
Member

Choose a reason for hiding this comment

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

This will cause a panic if a user doesn't provide a logger (whereas the previous code would not). Can we initialise a logr.Discard() here to pass to the Manager if it is nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's already a check for nil opts.Log above on line 102 https://github.com/cert-manager/csi-lib/pull/19/files#diff-f679b61276b44dc3f4712950536707c0389ee4c536fc29837c0202c6e81c1bbfR102

@munnerz
Copy link
Member

munnerz commented Feb 2, 2022

/test pull-cert-manager-csi-lib-verify

@wallrj
Update to cert-manager v1.7.1
714937d 
Signed-off-by: Richard Wall <[email protected]>
@wallrj wallrj changed the title WIP: Update to cert-manager 1.7 Update to cert-manager 1.7 Feb 18, 2022
@jetstack-bot jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 18, 2022
@wallrj
Copy link
Member Author

wallrj commented Feb 18, 2022

/unhold because we've updated to the latest 1.7.1 library and update the testing repo to use Go 1.17.

@wallrj wallrj requested a review from munnerz February 18, 2022 15:46
@munnerz
Copy link
Member

munnerz commented Mar 29, 2022

/unhold
/lgtm
/approve

@jetstack-bot jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 29, 2022
@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2022
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz, wallrj

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

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2022
@jetstack-bot jetstack-bot merged commit 34ffa38 into cert-manager:main Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@munnerz munnerz Awaiting requested review from munnerz

Assignees

@munnerz munnerz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wallrj @munnerz @jetstack-bot