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

Enable GOPROXY in default preset #13648

Merged
merged 2 commits into from
Jul 29, 2019
Merged

Enable GOPROXY in default preset #13648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b351a57
Enable GOPROXY in default preset
cblecker Jul 29, 2019
246b49c
Remove undeeded GOPROXY from pull-kubernetes-dependencies-canary
cblecker Jul 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions config/jobs/kubernetes-security/generated-security-jobs.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3737,10 +3737,6 @@ presubmits:
env:
- name: WHAT
value: vendor vendor-licenses
- name: GOPROXY
value: https://proxy.golang.org
- name: GOSUMDB
value: sum.golang.org
image: gcr.io/k8s-testimages/kubekins-e2e:v20190726-d319b3f-master
name: main
resources: {}
Expand Down
4 changes: 0 additions & 4 deletions config/jobs/kubernetes/sig-testing/dependencies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,6 @@ presubmits:
# Space separated list of the checks to run
- name: WHAT
value: "vendor vendor-licenses"
- name: GOPROXY
value: https://proxy.golang.org
- name: GOSUMDB
value: sum.golang.org
annotations:
testgrid-create-test-group: 'true'
- name: pull-kubernetes-godeps
Expand Down
4 changes: 4 additions & 0 deletions prow/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,3 +635,7 @@ presets:
hostPath:
path: /sys/fs/cgroup
type: Directory
# enable GOPROXY by default
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 think we support a 'default' preset that applies to all jobs implicitly and I'm not sure that we'd want to since that would be pretty opaque to job authors.

test-infra/prow/config/jobs.go

Lines 42 to 45 in c042e20

func mergePreset(preset Preset, labels map[string]string, containers []v1.Container, volumes *[]v1.Volume) error {
for l, v := range preset.Labels {
if v2, ok := labels[l]; !ok || v2 != v {
return nil

Copy link
Member

Choose a reason for hiding this comment

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

FWIW:

  • presets upstream are meant to be opaque for injecting environment (primarily credential) details
  • this setting only affects jobs that use go modules directly from within the container (not bazel or docker in docker)
  • in many settings this would be configured with a default on the machine (EG a corporate GOPROXY)

Setting this by default across the board makes sense, I did mention in previous discussion that the prow owners may not be OK with that though 🤷‍♂

Copy link
Member Author

Choose a reason for hiding this comment

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

I can confirm that the code as it is today will apply this "default" preset to every job (CI actually errors if this is set in the job spec because it merges and detects the duplicate).

+1 to everything Ben said. I can't think of any job on prow.k8s.io that wouldn't either benefit from this being set, or disregard it as a noop.

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with this, but let's make sure it's documented:

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #13665 for follow up

- env:
- name: GOPROXY
value: "https://proxy.golang.org"