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

Avoid rewriting template values, mark them as const #1190

Merged

Conversation

@justinsb
Copy link
Contributor

justinsb commented Nov 11, 2019

Fix a place where we rewrote a shared template value and potentially
made it unsafe for concurrent / repeated evaluation. Also mark the
templates as const to have the compiler help us stick to it!

justinsb added 2 commits Nov 11, 2019
Make them constant and use a local var.
To prevent the previous problem from being introduced elsewhere.
@justinsb justinsb force-pushed the justinsb:dont_rewrite_shared_vars branch from cda5ed3 to 7b632b2 Nov 12, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 12, 2019

@justinsb: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubebuilder-e2e 7b632b2 link /test pull-kubebuilder-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Contributor

camilamacedo86 left a comment

The changes shows good. 🥇
/lgtm /approve this one for me

@DirectXMan12 this one IMHO could be flagged to be merged when passing in the CI
Note that it issues has no relation with.

@camilamacedo86

This comment has been minimized.

Copy link
Contributor

camilamacedo86 commented Nov 12, 2019

/assign @DirectXMan12

@camilamacedo86

This comment has been minimized.

Copy link
Contributor

camilamacedo86 commented Nov 12, 2019

/assign @camilamacedo86

@Adirio
Adirio approved these changes Nov 12, 2019
Copy link
Contributor

Adirio left a comment

Just one small nit: the files where you have removed the extra blank spaces at the end, is this to normalize all of them? So that none of them have an extra blank line after it.

@DirectXMan12

This comment has been minimized.

Copy link
Contributor

DirectXMan12 commented Nov 14, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 14, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 14, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, justinsb

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

@DirectXMan12

This comment has been minimized.

Copy link
Contributor

DirectXMan12 commented Nov 14, 2019

Just one small nit: the files where you have removed the extra blank spaces at the end, is this to normalize all of them? So that none of them have an extra blank line after it.

I think gofmt may do this automatically in some cases

@k8s-ci-robot k8s-ci-robot merged commit 0742025 into kubernetes-sigs:master Nov 14, 2019
4 of 6 checks passed
4 of 6 checks passed
pull-kubebuilder-e2e Job failed.
Details
tide Not mergeable. Needs approved, lgtm labels.
Details
cla/linuxfoundation justinsb authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/kubebuilder/deploy-preview Deploy preview ready!
Details
pull-kubebuilder-test Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.