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

Tags: backport #928 to eight #1502

Merged
merged 5 commits into from Feb 1, 2015
Merged

Tags: backport #928 to eight #1502

merged 5 commits into from Feb 1, 2015

Conversation

jaredgrubb
Copy link
Member

This is a backport to eight of the following change:

commit 0d22261
Author: Jared Grubb jaredgrubb@gmail.com
Date: Mon Feb 10 12:53:43 2014 -0800

Fix #928: Allow builder to be associated with multiple tags

For the most part, this patch is identical to the one I contributed to nine. However, the Status parts of the patch (eg, waterfall) are all brand new, since Status does not exist in this form in Nine.

I'm sure there's going to be some issues, as I had some questions on parts of this patch (I'll write them inline below).
Also, docs arent ported and notes are not updated. I'll do that next, but wanted to get this up for comment.

@sa2ajj sa2ajj added this to the eight milestone Jan 23, 2015
@sa2ajj
Copy link
Contributor

sa2ajj commented Jan 23, 2015

(just 29 files... easy to review...)

jaredgrubb
jaredgrubb reviewed Jan 23, 2015
View reviewed changes
master/buildbot/config.py
error("builder '%s': category must be a string" % (name,))
tags = [category]

self.category = category # Set this for legacy reasons
Copy link
Member Author

Choose a reason for hiding this comment

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

This line was not part of the Nine review. I added it just in case subclasses had come to depend on it being there, but maybe that's not a concern.

@sa2ajj
Copy link
Contributor

sa2ajj commented Jan 23, 2015

LGTM

@sa2ajj
Copy link
Contributor

sa2ajj commented Jan 23, 2015

pyflakes error:

master/buildbot/status/web/waterfall.py:325: import 'builder' from line 32 shadowed by loop variable

@sa2ajj
Copy link
Contributor

sa2ajj commented Jan 27, 2015

@jaredgrubb do you think it will be mergeable by this weekend?

@jaredgrubb
Copy link
Member Author

Ok, I think I addressed all the comments. Also, I have updated the docs and release notes. (Note that when I did 'make docs', it failed, but it seemed to fail for other reasons).

console.py: remove a purported chromium workaround from 0.7
0d45379 
It's not clear what this code was intended to do and not sure if it should even still work like that
@sa2ajj
Copy link
Contributor

sa2ajj commented Feb 1, 2015

Looks like a pep-8 error:

master/buildbot/status/tinderbox.py:69:33: E261 at least two spaces before inline comment
categories=None # deprecated, use tags instead
^

master/buildbot/status/web/console.py:294:1: W293 blank line contains whitespace
^

master/buildbot/status/web/console.py:300:25: E225 missing whitespace around operator
if len(tags)==1:
^

master/buildbot/test/unit/test_status_mail.py:189:5: E303 too many blank lines (2)
def test_init_enforces_categories_and_builders_are_mutually_exclusive(self):
^

1 E225 missing whitespace around operator
1 E261 at least two spaces before inline comment
1 E303 too many blank lines (2)
1 W293 blank line contains whitespace

@jaredgrubb
Copy link
Member Author

Fixed two of the pep8 issues (the other two were removed in that chromium patch).

@sa2ajj sa2ajj removed the needs work label Feb 1, 2015
@sa2ajj
Copy link
Contributor

sa2ajj commented Feb 1, 2015

Thank you very much! :)

Waiting for Travis...

sa2ajj pushed a commit that referenced this pull request Feb 1, 2015
Merge pull request #1502 from jaredgrubb/BuilderTags
760357d 
Tags: backport #928 to eight
@sa2ajj sa2ajj merged commit 760357d into buildbot:eight Feb 1, 2015
@sa2ajj
Copy link
Contributor

sa2ajj commented Feb 1, 2015

We seem to have an error: http://buildbot.buildbot.net/console

@sa2ajj
Copy link
Contributor

sa2ajj commented Feb 1, 2015

TRAC-3179 has a better traceback.

sa2ajj
sa2ajj reviewed Feb 1, 2015
View reviewed changes
master/buildbot/status/web/console.py
# Display the boxes by category group.
for category in categories:
# Display the boxes by tag group.
for tag in tags:
Copy link
Contributor

Choose a reason for hiding this comment

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

Inside this for.. loop there was a variable tag used, which overwrites the value assigned here.

@sa2ajj
Copy link
Contributor

sa2ajj commented Feb 1, 2015

#1514 partially fixes things.

sa2ajj pushed a commit that referenced this pull request Feb 1, 2015
Merge pull request #1514 from sa2ajj/partial-fix
47bff6a 
Partial fix for #1502
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
eight
Projects
None yet
Milestone
eight
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jaredgrubb @sa2ajj