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

Implicit self-approval and consider github review state by default #10435

Merged
merged 1 commit into from
Jan 10, 2019
Merged

Implicit self-approval and consider github review state by default #10435

merged 1 commit into from
Jan 10, 2019

Conversation

fejta
Copy link
Contributor

@fejta fejta commented Dec 14, 2018

Maintain backwards compatibility with current config values (which have the opposite defaults) for 6 months.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 14, 2018
@k8s-ci-robot k8s-ci-robot added the area/prow Issues or PRs related to prow label Dec 14, 2018
@fejta
Copy link
Contributor Author

fejta commented Dec 15, 2018

/assign @cblecker

@cblecker
Copy link
Member

lololol.. you're just itching to swap out the defaults! will review this next week when my brain isn't so fried.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2018
@cjwagner
Copy link
Member

This is a breaking config change so it should be mentioned in the announcements: https://github.com/kubernetes/test-infra/blob/master/prow/ANNOUNCEMENTS.md

@stevekuznetsov
Copy link
Contributor

Why?

@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Jan 7, 2019
@fejta
Copy link
Contributor Author

fejta commented Jan 7, 2019

should be mentioned in the announcements

Done

Why?

Because we should have the right defaults

@fejta
Copy link
Contributor Author

fejta commented Jan 9, 2019

Who wants to review this? 👯‍♂️ happy to assign new people too

cjwagner
cjwagner reviewed Jan 9, 2019
View reviewed changes
Copy link
Member

@cjwagner cjwagner left a comment

Choose a reason for hiding this comment

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

Mostly LGTM except for the race on the last warning time variables.

prow/plugins/plugins.go Outdated
func warnDeprecated(last *time.Time, freq time.Duration, msg string) {
now := time.Now()
if now.Sub(*last) > freq {
*last = now
Copy link
Member

Choose a reason for hiding this comment

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

This isn't thread safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@stevekuznetsov
Copy link
Contributor

But why are these the right defaults?

@fejta
Copy link
Contributor Author

fejta commented Jan 9, 2019 •
edited
Loading

Because by default, one expects:

  • leaving an approving github review will... approve the PR. and
  • to personally approve of own's own PR before creating it

It is fine to override this behavior if you want to ignore the github review state for some reason, or your org has a habit of creating PRs they disagree with for some weird reason, but people deserve a better OOB experience

@fejta
Copy link
Contributor Author

fejta commented Jan 9, 2019

Passes gofmt now

@fejta fejta closed this Jan 9, 2019
@fejta fejta reopened this Jan 9, 2019
cblecker
cblecker approved these changes Jan 10, 2019
View reviewed changes
Copy link
Member

@cblecker cblecker left a comment

Choose a reason for hiding this comment

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

LGTM. WIll leave lgtm to @cjwagner to verify thread safe fix is good.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2019
cjwagner
cjwagner approved these changes Jan 10, 2019
View reviewed changes
Copy link
Member

@cjwagner cjwagner left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold cancel

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

LGTM label has been added.

Git tree hash: 1aa69e694865c14e5c33b980aba8290482fcea61

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, cjwagner, fejta

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 01cac18 into kubernetes:master Jan 10, 2019
@cjwagner cjwagner mentioned this pull request Jan 12, 2019
Closed
@fejta fejta deleted the def branch January 15, 2019 19:42
@spiffxp spiffxp mentioned this pull request Jan 16, 2019
Closed
@grantr grantr mentioned this pull request Jun 13, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cblecker cblecker cblecker approved these changes

@cjwagner cjwagner cjwagner approved these changes

@stevekuznetsov stevekuznetsov Awaiting requested review from stevekuznetsov

Assignees

@cblecker cblecker

@cjwagner cjwagner

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/prow Issues or PRs related to prow 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. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fejta @cblecker @cjwagner @stevekuznetsov @k8s-ci-robot