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

Rename all controller.go to be easily distinguishable in logs #13699

Merged
merged 1 commit into from
May 3, 2017

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Apr 10, 2017

We need this for ease of debugging, and it should be merged for 3.6, still.

Since I don't want to mess people yet, I'll wait for at least the rebase to land. @csrwng I've heard you're re-factoring build controllers so if you could pick the build part that will be also great and won't interfere with your changes @bparees mentioned.

@sttts @ncdc @smarterclayton @liggitt @deads2k @mfojtik @knobunc @danwinship fyi

Currently blocked by #13653

@soltysh soltysh added this to the 1.6.0 milestone Apr 10, 2017
@soltysh
Copy link
Contributor Author

soltysh commented Apr 10, 2017

[test]

@deads2k
Copy link
Contributor

deads2k commented Apr 10, 2017

I like the idea of renaming them.

@bparees
Copy link
Contributor

bparees commented Apr 10, 2017

+1 on renaming them and +1 on letting us rename our own controllers :)

@deads2k
Copy link
Contributor

deads2k commented Apr 10, 2017

+1 on renaming them and +1 on letting us rename our own controllers :)

Didn't someone (who can remain nameless), force us to make all the filenames the same? Something about go-like instead of java-like maybe?

@soltysh
Copy link
Contributor Author

soltysh commented Apr 10, 2017

Didn't someone (who can remain nameless), force us to make all the filenames the same? Something about go-like instead of java-like maybe?

That makes debugging any remote issue with controllers almost impossible. That's because with the glog limitation we only see the go filename, but no path to it. K8S already did that change, if we need another argument of this sort ;)

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2017
@soltysh soltysh self-assigned this Apr 20, 2017
@soltysh soltysh force-pushed the rename_controllers branch from a9e3755 to c99ee02 Compare May 2, 2017 10:19
@soltysh soltysh changed the title [DO_NOT_MERGE] Rename all controller.go to be easily distinguishable in logs Rename all controller.go to be easily distinguishable in logs May 2, 2017
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to c99ee02

@soltysh
Copy link
Contributor Author

soltysh commented May 2, 2017

Since the rebase landed, I'd like to push this forward as well. Any objections? @bparees since you complained a bit 😉

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2017
@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1076/) (Base Commit: 0bf8c01)

@bparees
Copy link
Contributor

bparees commented May 2, 2017

It's going to merge conflict some people but it shouldn't be too bad. Go for it. I'm certainly in favor of the rename in general.

@deads2k
Copy link
Contributor

deads2k commented May 2, 2017

[merge]

@soltysh
Copy link
Contributor Author

soltysh commented May 2, 2017

Flakes #12072 and #13985

@soltysh
Copy link
Contributor Author

soltysh commented May 2, 2017

It's going to merge conflict some people but it shouldn't be too bad. Go for it. I'm certainly in favor of the rename in general.

I know, myself included, since I'm modifying the image controller, switching to shared informers.

@ncdc
Copy link
Contributor

ncdc commented May 2, 2017

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to c99ee02

@openshift-bot
Copy link
Contributor

openshift-bot commented May 2, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/535/) (Base Commit: 7a4496d) (Image: devenv-rhel7_6199)

@openshift-bot openshift-bot merged commit 1db21af into openshift:master May 3, 2017
@soltysh soltysh deleted the rename_controllers branch May 3, 2017 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants