Throw an error rather than just logging for missing SLS, Fix #5998 #8028
Conversation
Throw an error rather than just logging for missing SLS, Fix #5998
|
What brought this up? I don't want a ton of our users' states to just start failing with the next upgrade! I would like to have discussed this one before merging. |
|
@basepi, lets revert this for now and look into making a better solution. I would like to start displaying an errors out state in the return from the state run that this happened rather than failing hard like this |
|
I revert my statement! This is in render_highstate and is evaluating the top file, not the include statement, this is the right approach, thanks for fixing this @basepi and @cachedout ! |
|
Yes, last time only one person complained about the change, and even had a workaround. But if states defined in top.sls don't exist, I can't imagine wanting to just ignore that problem and continue blindly. @cachedout and I want to talk to you when you're in office about some changes that would expose more information to callers, such as the CLI, so we could display information like this more strategically. You're right, though, I shouldn't have just merged this one. @cachedout and I had had an in-depth discussion about it and were very sure about the change, but I didn't think about the fact that you weren't involved at all! I'll be more careful in the future. |
|
no worries, I over reacted, this is good code |
Looking back at the arguments for and against (ref #5430), @cachedout and I decided that continuing without error in case of a missing SLS was much more dangerous and problematic than having to use the workaround defined in #5430. If an SLS is not found, an error will not be output and the staterun will abort.