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

Upgrade ExternalDNS to go 1.19 #3152

Merged
merged 9 commits into from
Nov 30, 2022
Merged

Upgrade ExternalDNS to go 1.19 #3152

merged 9 commits into from
Nov 30, 2022

Conversation

Raffo
Copy link
Contributor

@Raffo Raffo commented Nov 11, 2022

Description

This PR updates ExternalDNS to go 1.19. Replaces #3131.

@Raffo
upgrades ExternalDNS to go 1.19
7a09c51 
Signed-off-by: Raffaele Di Fazio <difazio.raffaele@gmail.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 11, 2022
@Raffo Raffo mentioned this pull request Nov 11, 2022
Closed
@szuecs
Copy link
Contributor

szuecs commented Nov 13, 2022

Interesting linter error
I would suggest to disable golint because it's abandoned since long.
I would expect some godoc errors, but maybe we have not so much indentation based docs.

@Raffo
Copy link
Contributor Author

Raffo commented Nov 23, 2022

The tests are failing because of a difference in the ExampleSameEndpoints test which I will be debugging:

image

@Raffo
fixes testutil comparison
2e345d3 
Signed-off-by: Raffaele Di Fazio <difazio.raffaele@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 27, 2022
@Raffo
disable golint
c36d3bc 
Signed-off-by: Raffaele Di Fazio <difazio.raffaele@gmail.com>
@Raffo
bump golangici-lint
0aa03e8 
Signed-off-by: Raffaele Di Fazio <difazio.raffaele@gmail.com>
@Raffo
remove deprecated linters and revive
70ce5ec 
Signed-off-by: Raffaele Di Fazio <difazio.raffaele@gmail.com>
@Raffo
Merge branch 'master' into raffo/upgrade-to-go-1.19
de53d2f
@Raffo
remove other linters to make it pass
8ec06e8 
Signed-off-by: Raffaele Di Fazio <difazio.raffaele@gmail.com>
@Raffo
remove gosimple
76e217b 
Signed-off-by: Raffaele Di Fazio <difazio.raffaele@gmail.com>
@Raffo Raffo added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 27, 2022
szuecs
szuecs reviewed Nov 27, 2022
View reviewed changes
.golangci.yml
- govet
- ineffassign
- misspell
- rowserrcheck
- staticcheck
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm that's the linter I would not drop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we updated it to the last version, there are a lot of issues that were ignored. I'm dropping it only to make the 1.19 update possible and then work on #3194 to reintroduce that and the other ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

@Raffo
Copy link
Contributor Author

Raffo commented Nov 28, 2022

Tests are broken because they are broken on master. Give info to the author of the failing tests in #3180 (comment), will revert this week if they are not fixed.

@Raffo
Copy link
Contributor Author

Raffo commented Nov 29, 2022

This is ready to go @szuecs @njuettner

njuettner
njuettner approved these changes Nov 30, 2022
View reviewed changes
Copy link
Member

@njuettner njuettner left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: njuettner, Raffo

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 merged commit cfce744 into master Nov 30, 2022
@mloiseleur mloiseleur mentioned this pull request Jun 9, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szuecs szuecs szuecs left review comments

@njuettner njuettner njuettner approved these changes

@seanmalloy seanmalloy Awaiting requested review from seanmalloy

Assignees

@njuettner njuettner

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/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Raffo @szuecs @k8s-ci-robot @njuettner