Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix `BadConfigurationStep` and return the `try/except` to `DynamicServoFactory` #444
Conversation
Because this step was not getting fully initialized, an exception was getting thrown by Buildbot before it attempted to use this step, hence making it useless for its original purpose of displaying a traceback from an earlier exception thrown while making Steps from the steps.yml.
We used to use a regular `print` statement for logging here, but that output was not going anywhere we could find. Buildbot has a native buildbot.config.error() function that we could use, but that would mean that Buildbot would completely fail to start in case of a misconfiguration, blocking other jobs from being run on builders that are still correctly configured (e.g. when testing out a new builder). Instead, continue to handle and log step misconfiguration errors at runtime, but use native Buildbot logs to do so, generating a log file that is accessible in the Buildbot web UI.
| dynamic_steps = [self.make_step(command) for command in commands] | ||
| except Exception as e: # Bad step configuration, fail the build | ||
| dynamic_steps = [BadConfigurationStep(e)] | ||
|
|
This comment has been minimized.
This comment has been minimized.
larsbergstrom
Jul 21, 2016
Contributor
Is this too many blank lines? Otherwise, all looks good - r=me
This was recently removed due to other bugs that were causing our error recovery reporting to be useless, but now that those are fixed this can be returned to allow Buildbot to start even if certain builders have bad configurations (e.g. a new builder that is getting up to speed), allowing other changes to land in the meantime.
aneeshusa
force-pushed the
aneeshusa:call-super-constructor-for-bad-configuration-step
branch
from
b9fec2d
to
43a01cb
|
Removed the extra line of whitespace. This is mostly based on reading the docs but I don't have a good way to verify locally, so I'd like to test this ASAP when we deploy it to make sure it works properly, instead of finding out that it's broken while dealing with other issues (e.g. Windows). |
|
@bors-servo r+ (sorry, I had meant for this to be r=me after whitespace removal and then missed the update!) |
|
|
bors-servo
added a commit
that referenced
this pull request
Jul 23, 2016
…ration-step, r=larsbergstrom Fix `BadConfigurationStep` and return the `try/except` to `DynamicServoFactory` See the commit messages for details and more rationale. Follow-up to #441. r? @larsbergstrom @edunham <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/444) <!-- Reviewable:end -->
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
aneeshusa commentedJul 21, 2016
•
edited by larsbergstrom
See the commit messages for details and more rationale.
Follow-up to #441. r? @larsbergstrom @edunham
This change is