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.

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

Update Go version to 1.19 #2428

Merged
merged 7 commits into from
Oct 24, 2022
Merged

Update Go version to 1.19 #2428

merged 7 commits into from
Oct 24, 2022

Conversation

4lexvav
Copy link
Contributor

@4lexvav 4lexvav commented Oct 18, 2022

No description provided.

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

Couldn't verify .devcontainer/devcontainer.json because I'm not familiar with the VS Code IDE. Otherwise, the change looks good to me with the exception of the comment found in endpoints/cookie_sync_test.go shown below:

1829 // ErrReader returns an io.Reader that returns 0, err from all Read calls. This is added in
1830 // Go 1.16. Copied here for now until we switch over.
1831 func ErrReader(err error) io.Reader {
1832     return &errReader{err: err}
1833 }
endpoints/cookie_sync_test.go

Can we modify or, if needed, remove?

@@ -6,7 +6,7 @@
"dockerfile": "Dockerfile",
"args": {
// Update the VARIANT arg to pick a version of Go
"VARIANT": "1.16",
"VARIANT": "1.19",
Copy link
Contributor

Choose a reason for hiding this comment

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

@AlexBVolcy I don't use VS Code so I can't assert this line to be correct

@guscarreon guscarreon self-assigned this Oct 18, 2022
@bsardo bsardo self-requested a review October 19, 2022 02:27
Copy link
Contributor

@AlexBVolcy AlexBVolcy left a comment

Choose a reason for hiding this comment

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

This looks good. I'm just waiting on @SyntaxNode to verify .devcontainer/devcontainer.json before approval

@4lexvav
Copy link
Contributor Author

4lexvav commented Oct 20, 2022

@guscarreon thanks for bringing my attention to the comment in file endpoints/cookie_sync_test.go, I removed this code and changed its usage to ErrReader from the standard library.

@SyntaxNode
Copy link
Contributor

This looks good. I'm just waiting on @SyntaxNode to verify .devcontainer/devcontainer.json before approval

The tests failed. Turns out, Go 1.18 added a new feature where the binary includes information about the git repository. This breaks in the dev container since git isn't really available. I've implemented (and tested) a fix along with updates to now-outdated config.

Please update to this file:

// For format details, see https://aka.ms/vscode-remote/devcontainer.json or this file's README at:
// https://github.com/microsoft/vscode-dev-containers/tree/v0.112.0/containers/go
{
	"name": "Go",
	"build": {
		"dockerfile": "Dockerfile",
		"args": {
			// Update the VARIANT arg to pick a version of Go
			"VARIANT": "1.19",
			// Options
			"INSTALL_NODE": "false",
			"NODE_VERSION": "lts/*"
		}
	},
	"containerEnv": {
		"GOPRIVATE": "${localEnv:GOPRIVATE}",
		"GOFLAGS": "-buildvcs=false",
		"PBS_GDPR_DEFAULT_VALUE": "0"
	},
	"runArgs": [ "--cap-add=SYS_PTRACE", "--security-opt", "seccomp=unconfined" ],

	// Set *default* container specific settings.json values on container create.
	"settings": {
		"terminal.integrated.profiles.linux": {
			"bash": {
				"path": "/usr/bin/bash"
			}
		},
		"terminal.integrated.defaultProfile.linux": "bash",
		"go.toolsManagement.checkForUpdates": "off",
		"go.gopath": "/go",
	},

	// Add the IDs of extensions you want installed when the container is created.
	"extensions": [
		"golang.Go",
		"ms-azuretools.vscode-docker",
		"redhat.vscode-xml",
		"redhat.vscode-yaml",
		"eamodio.gitlens",
	],

	// Use 'forwardPorts' to make a list of ports inside the container available locally.
	"forwardPorts": [8000,8001,6060],

	// Use 'postCreateCommand' to run commands after the container is created.
	"postCreateCommand": "mkdir ~/.ssh; ssh-keyscan github.com > ~/.ssh/known_hosts",

	// Uncomment to connect as a non-root user. See https://aka.ms/vscode-remote/containers/non-root.
	"remoteUser": "vscode"
}

@AlexBVolcy
Copy link
Contributor

Waiting for update based on @SyntaxNode comment before approval.

@4lexvav 4lexvav requested review from AlexBVolcy and removed request for bsardo October 21, 2022 09:21
Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

Thank you for addressing our feedback @4lexvav . Changes look good to me

@bsardo bsardo merged commit d162aa9 into prebid:master Oct 24, 2022
@4lexvav 4lexvav deleted the bump-go-version branch November 3, 2022 14:15
shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022
thebraveio pushed a commit to thebraveio/prebid-server that referenced this pull request Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants