-
Notifications
You must be signed in to change notification settings - Fork 46
Introduce results collection with Buildbot #522
Conversation
The trial I ran last night surfaced a number of issues with the final
There are a few other changes that need to be made to integrate with the
I'll be addressing these issues with new commits to this pull request shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor nits. Some are serious, others are not. I'll let you determine which are which ;)
run/README.md
Outdated
3. Open a terminal and navigate to the directory containing this text file | ||
4. Run the following command: | ||
|
||
`ansible-playbook -i inventory/production playbook.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing closing backtick (either add, or remove both and rely on indentation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
31356437386434326330643736653262636238643261616330343432326432626262376533646133 | ||
64613530623536373936663836383430666536356537326331663430363663306639313664363834 | ||
35363035353636616166303262346661353137363139626662663332356438366330343737633535 | ||
336439363936303136353532343039343034 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect there might be an overallocation of the number 3 in this data... can you confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I had a few 3s left over from an old project, and I thought it would be okay to use them here
- emacs-nox | ||
- firefox | ||
- git | ||
- google-chrome-stable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: we acknowledge that inclusion of google-chrome-stable and firefox are temporary measures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true, and it wouldn't hurt to document that intention with a comment, either
39323032613636356336363863386234353163396635666430386266303634643531623135396138 | ||
65363333376463643933616265666239386339636338383665393339643139396530393930326264 | ||
31383632393163333030656535323036316565336633373964613461303363636262623933366235 | ||
3564303937653961333139356261653963363934316362363131 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is substantially shorter than the previous—are you sure that's correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed so, but now that you ask, I'm not so sure.
gs_http_results_url=None, | ||
summary_filename=None, | ||
summary_http_url=None, | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sad-face emoji was strangely appropriate following all of those arguments for one function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A sure sign of technical debt. Note that this patch, fresh as it is, has its own problems--see the main
function of upload-wpt-results.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂
run/src/master/master.cfg
Outdated
####### CHANGESOURCES | ||
|
||
# the 'change_source' setting tells the buildmaster how it should find out | ||
# about source code changes. Here we point to the buildbot version of a python hello-world project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point these comments should be updated (wherever applicable) to reflect reality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and done
run/src/master/master.cfg
Outdated
workdir='gitpoller-workdir', | ||
branch='master', | ||
pollAtLaunch=True, | ||
pollinterval=300) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pollinterval
is deprecated. Update to pollInterval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're like a bloodhound for this stuff. See also buildbot/buildbot#4011
run/src/master/master.cfg
Outdated
chunked_steps.append(WPTChunkedStep(schedulerNames=['chunked'], | ||
platform=spec, | ||
doStepIf=partial(filter_build, speed), | ||
total_chunks=100)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: make a total_chunks
variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You got it
run/src/master/master.cfg
Outdated
# failed builds are manually re-tried via the web interface. | ||
steps.Git(repourl='git://github.com/w3c/web-platform-tests'), | ||
steps.SetPropertyFromCommand(name='Retrieve commit date of latest revision', | ||
command=['git', 'log', '-1', '--format=%cd', '--date=iso-strict'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment explaining why this code is capturing the date of the commit now, instead of at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
64326632313065306237336339373361616130366139653530396534363063353535616530303964 | ||
33626264633034383132623361353762363939633032663364633938323630316131626363376566 | ||
36353337363438346537386335306535343765373565373430366338653033366336393665323534 | ||
383839633333646530356434626530623530 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the rest of the code that's missing from the second file I reviewed? IDK...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be. Honestly, I have a hard time keeping track of these really long number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂🤣
As far as I can tell, the travis failure is unrelated to this changeset. |
run/src/master/master.cfg
Outdated
@@ -215,7 +215,7 @@ upload_factory = util.BuildFactory([ | |||
'--os-version', util.Property('os_version'), | |||
'--wpt-revision', util.Property('revision'), | |||
'--wpt-revision-date', util.Property('revision_date'), | |||
'--bucket-name', 'wptd2', | |||
'--bucket-name', 'wptd', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Thanks, @rwaldron. I've squashed most of the commits, but I'm retaining a few that have to do with file re-organization. I think it makes sense to keep these distinct because they would only add noise to the commit that introduces the new system. Since the intermediary commits do not describe valid states of the code base, this branch should be merged with a merge commit. The original version of this branch is available here: |
@jugglinmike thanks! |
Although it's preferable to submit changes in piecemeal fashion, this change to
the infrastructure is difficult to break down in a meaningful way. I plan to
walk @rwaldron through the patch in person tomorrow. I'm happy to do the same
for any interested party using some video conferencing/desktop sharing system.
In addition, the new system is currently deployed so
that we have a better sense of how it actually functions.