Skip to content

Tools: Persist config errors until validation occurs #7277

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
Merged
merged 4 commits into from
Jul 5, 2018
Merged

Tools: Persist config errors until validation occurs #7277

merged 4 commits into from
Jul 5, 2018

Conversation

theotherjimmy
Copy link
Contributor

Description

As @geky pointed out in #5416, Configuration errors in
"not-bottom-level" library configurations, a library that's not part of
the last feature set added, do not show errors. This was because
processing new library configurations cleared errors. This PR removes
the clearing behavior.

Resolves #5416

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@0xc0170 0xc0170 requested a review from geky June 20, 2018 14:31
geky
geky previously approved these changes Jun 20, 2018
View reviewed changes
Copy link
Contributor

@geky geky left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

0xc0170
0xc0170 previously approved these changes Jun 20, 2018
View reviewed changes
cmonr
cmonr previously approved these changes Jun 21, 2018
View reviewed changes
@cmonr
Copy link
Contributor

cmonr commented Jun 21, 2018

/morph build

@cmonr
Copy link
Contributor

cmonr commented Jun 21, 2018

@theotherjimmy This needs to pass Travis CI before we can start Jenkins CI.

@theotherjimmy theotherjimmy dismissed stale reviews from cmonr, 0xc0170, and geky via 66acb17 June 21, 2018 16:15
@theotherjimmy
Copy link
Contributor Author

@cmonr That should do it.

cmonr
cmonr suggested changes Jun 21, 2018
View reviewed changes
Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Just have a question about a print statement.

tools/config/__init__.py Outdated
@@ -795,14 +808,11 @@ def _process_config_and_overrides(self, data, params, unit_name, unit_kind):
setattr(self.target, attribute, val)
continue
else:
if "lib2.test" == name:
print("HERE {}".format(unit_name))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this suppose to be in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IT's supposed to be "THERE", actually :D

@theotherjimmy theotherjimmy force-pushed the error-on-unknown-config branch from 7034a23 to 241b786 Compare June 22, 2018 14:12
cmonr
cmonr previously approved these changes Jun 22, 2018
View reviewed changes
@cmonr
Copy link
Contributor

cmonr commented Jun 22, 2018

LGTM, but still needs to pass Travis.

@theotherjimmy theotherjimmy force-pushed the error-on-unknown-config branch from 241b786 to 2a9923f Compare June 28, 2018 15:26
@cmonr
Copy link
Contributor

cmonr commented Jul 3, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jul 3, 2018

Build : SUCCESS

Build number : 2521
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7277/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Jul 3, 2018

Exporter Build : SUCCESS

Build number : 2166
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7277/

@mbed-ci
Copy link

mbed-ci commented Jul 3, 2018

Test : FAILURE

Build number : 2283
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7277/2283

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 4, 2018

/morph test

@mbed-ci
Copy link

mbed-ci commented Jul 4, 2018

Test : SUCCESS

Build number : 2284
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7277/2284

@cmonr cmonr merged commit fab0849 into ARMmbed:master Jul 5, 2018
@cmonr
Copy link
Contributor

cmonr commented Jul 16, 2018

Well this is annoying. Looks like this became dependent on changes going into 5.10: https://github.com/ARMmbed/mbed-os/pull/7133/files#diff-0f1a973b12e465163c3fe8f2d1f933c9R544

Re-tagging to 5.10 to get 5.9.3 PR functional.

@cmonr cmonr mentioned this pull request Jul 16, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmonr cmonr cmonr approved these changes

@geky geky geky left review comments

@0xc0170 0xc0170 0xc0170 left review comments

Assignees
No one assigned
Labels
release-version: 5.10.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@theotherjimmy @cmonr @mbed-ci @0xc0170 @geky