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

Ignore invalid display config files and boot normally. #3635

Closed

Conversation

tarek-y-ismail
Copy link
Contributor

Closes #3628

Adds a new exception type StaticDsiplayConfig::InvalidConfig, which is now used inside YamlFileDisplayConfig::load_config to signal a failure in parsing. When this exception is caught, we skip the rest of config parsing and the remaining code that depends on it and run as if --display-config=static=path/to/invalid/file.display was not provided.

@tarek-y-ismail tarek-y-ismail self-assigned this Oct 16, 2024
@tarek-y-ismail tarek-y-ismail requested a review from a team as a code owner October 16, 2024 08:27
Copy link
Collaborator

@Saviq Saviq left a comment

Choose a reason for hiding this comment

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

Let's make the new behaviour optional (off by default)?

Keep the current constructor so we don't break ABI, but add a new one with a bool exit_on_error=true?

This way, Frame we can make to start up regardless of display config issues.

Also, in StaticDisplayConfig we could then log an error for the less critical bits and ignore / use defaults for that particular element rather than bail completely.

Copy link
Collaborator

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

Just a note that I'm against the resolution suggested in the issue. (This implements that resolution: I'm not against the implementation)

@Saviq
Copy link
Collaborator

Saviq commented Oct 16, 2024

As discussed on the bug, maybe having a special case (and that could be default) for the file being empty would also do.

@AlanGriffiths
Copy link
Collaborator

Let's make the new behaviour optional (off by default)?

+1

Keep the current constructor so we don't break ABI, but add a new one with a bool exit_on_error=true?

Or use the "named constructor" idiom: StaticDisplayConfig:on_errors_use_default() is clearer than StaticDisplayConfig{true}/StaticDisplayConfig{false}

This way, Frame we can make to start up regardless of display config issues.

I'm still not convinced this is desirable

@AlanGriffiths
Copy link
Collaborator

Also, we should have tests

@tarek-y-ismail
Copy link
Contributor Author

Also, we should have tests

Lucky for me, we already do (the ones failing right now :)). I'll see if I can add any.

@tarek-y-ismail tarek-y-ismail force-pushed the MIRENG-740/static-display-file-resiliency branch from 902efbe to c924412 Compare October 16, 2024 09:08
@AlanGriffiths
Copy link
Collaborator

We decided on another approach

@AlanGriffiths AlanGriffiths deleted the MIRENG-740/static-display-file-resiliency branch October 16, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StaticDisplayConfig: unrecognized content prevents Mir starting up
3 participants