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

[#180030835] Upgrade from Go 1.12 to Go 1.17 #20

Merged
merged 4 commits into from
Oct 28, 2021
Merged

[#180030835] Upgrade from Go 1.12 to Go 1.17 #20

merged 4 commits into from
Oct 28, 2021

Conversation

AP-Hunt
Copy link
Member

@AP-Hunt AP-Hunt commented Oct 14, 2021

What

Upgrades from Go 1.12 to Go 1.17 across the board

How to review

Check tests pass. The Go versions themselves should be backwards compatible.

@AP-Hunt AP-Hunt force-pushed the 179506968_go_1.17 branch 2 times, most recently from c418a60 to 40c1afa Compare October 14, 2021 14:31
@risicle risicle force-pushed the 179506968_go_1.17 branch 2 times, most recently from 964a4e7 to 6ffe426 Compare October 25, 2021 12:26
@risicle
Copy link
Member

risicle commented Oct 25, 2021

Integration tests pass.

Since we don't appear to be running travis anymore I wonder if I should be converting this to a github action.

@risicle
Copy link
Member

risicle commented Oct 25, 2021

Ok did that, seems to work.

@risicle risicle changed the title [#179506968] Upgrade from Go 1.12 to Go 1.17 [#180030835] Upgrade from Go 1.12 to Go 1.17 Oct 25, 2021
@risicle
Copy link
Member

risicle commented Oct 27, 2021

Oh I've missed some references in the readme to the old project setup.

paroxp
paroxp approved these changes Oct 27, 2021
View reviewed changes
Copy link
Member

@paroxp paroxp left a comment

Choose a reason for hiding this comment

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

Code wise looks sane.

@risicle
Copy link
Member

risicle commented Oct 27, 2021 •
edited
Loading

Oh I've spotted a problem. My solution to the integration tests not working was to give the certificate a SAN, but

paas-rds-metric-collector/scripts/generate-cert-fixtures.sh

Line 62 in 0f7dcda

# Note: we should NOT add a SAN here

It would be lovely to figure out why. According to the commit message

  • The loggregator cert needs a CN of metron (and cannot have a SAN entry)

is this because the cert provided IRL won't have a SAN? If so, that's a problem.

@risicle
Copy link
Member

risicle commented Oct 27, 2021 •
edited
Loading

This looks like the problem the integration tests were having (certificate relies on legacy Common Name field, use SANs instead) is due to the golang upgrade past 1.15. 1.17 has additionally got rid of the GODEBUG=x509ignoreCN=0 hack that might have saved us. So if the test deployment doesn't work because modern CF still isn't giving us a cert with a SAN, we'll have to think again.

@risicle
Copy link
Member

risicle commented Oct 28, 2021

According to Andy, CF now does have SANs on all its certs. So this should be ok. I should still probably test it manually though.

@risicle
Copy link
Member

risicle commented Oct 28, 2021

Confirmed, works in dev02. Still I'm going to regenerate the new cert actually using the supplied script for consistency.

risicle and others added 4 commits October 28, 2021 15:04
@risicle
integration test fixtures: use SAN in loggregator server certificate
59db0b0 
required to placate modern golang. modern cf *does* now supply SANs
on its metron certs so we're good to do similar in our integration
tests
@risicle
integration test fixtures: update requested postgres to 12
c958b40 
9.5 is no longer offered by RDS
@AP-Hunt @risicle
Upgrade from Go 1.12 to Go 1.17
0feafcc 
update some dependencies in the process

switch to a forked pq-timeouts which works with modern pq versions

update integration tests to use go install for required executables,
which is the recommended way in go 1.17

get rid of vendor directory
@risicle
unit tests: fixup, convert travis configuration to github workflow
ff480d4 
this could still do with some more attention, e.g. test against
multiple db versions
@risicle
Copy link
Member

risicle commented Oct 28, 2021

Even though it technically doesn't need it, I think this should get a re-approval before I consider merging it.

@risicle risicle merged commit 6907ac8 into main Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paroxp paroxp paroxp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@AP-Hunt @risicle @paroxp