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

state.sls should bail out, when desired SLS does not exist #5998

Closed
pille opened this issue Jul 6, 2013 · 8 comments
Closed

state.sls should bail out, when desired SLS does not exist #5998

pille opened this issue Jul 6, 2013 · 8 comments
Assignees
Labels
Bug broken, incorrect, or confusing behavior

Comments

@pille
Copy link
Contributor

pille commented Jul 6, 2013

currently when i deploy a state manually and have a typo in state-name, this seems to be up-to-date. instead it should show an error, so i can notice my mistake.

pille@salt-master ~ % time sudo salt -v 'target*' state.sls test.tset-statte                                                                                                                                                                                
Executing job with jid 20130706130410285321
-------------------------------------------

Execution is still running on target
Execution is still running on target
target:

0.21s user 0.08s system 1% cpu 22.174 total
@basepi
Copy link
Contributor

basepi commented Jul 8, 2013

For a time we had it erroring out if the defined SLS doesn't exist. I think now we just do an error log and continue, because it was messing up some peoples' generic states.

@pille
Copy link
Contributor Author

pille commented Jul 8, 2013

would it be ok to show the error on std(err|out), when using --verbose?

@basepi
Copy link
Contributor

basepi commented Jul 9, 2013

Should be possible, I haven't looked at the --verbose code at all. We should definitely try to make it as transparent as possible.

@ghost ghost assigned cachedout Oct 17, 2013
@thatch45
Copy link
Contributor

@cachedout please verify the current behavior

@cachedout
Copy link
Contributor

@thatch45 Current behavior is as-specified by the author of the issue:

(master_env)root@master:/salt_mount# /salt_mount/scripts/salt minion1 state.sls notastate.nopeitisnot
minion1:

Summary
-----------
Succeeded: 0
Failed:   0
-----------
Total:    0

@basepi
Copy link
Contributor

basepi commented Oct 19, 2013

Yes, this is definitely not fixed. The plan as stated above is to add the error log to the verbose output if possible. We don't want to actually fail, because people want to be able to have generic states which have states that don't exist in them (can't find the previous conversation atm). But it would be nice to notify the user why no states were executed.

@cachedout
Copy link
Contributor

There are some architectural decisions that @thatch45 should weigh in on here.

Right now, the verbose flag filters down into the LocalClient class and has insights into jobs and the event system. However, to extend this to actually evaluate cases where a state isn't found would involve wiggling around in the event system some in order to define this condition in a return code for a job. (@thatch45, please correct me if I'm wrong here. If there's a way for the event system to easily handle this case, then that's clearly the way to go.)

A less invasive approach would be simply to log the event using the logging system, but I'm sympathetic to the case that @basepi is advocating that this might not be immediately obvious to the user running the command.

@basepi
Copy link
Contributor

basepi commented Oct 22, 2013

So I went back and found the argument for warning instead of bailing out: #5430

I think that continuing forward blindly and only warning in the log is the wrong behavior. So I'm going to add the error back in, rather than just logging it. Those who need "generic" states as defined in #5430 can use the workaround listed there.

@basepi basepi closed this as completed in 7f871f7 Oct 22, 2013
basepi added a commit that referenced this issue Oct 22, 2013
Throw an error rather than just logging for missing SLS, Fix #5998
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior
Projects
None yet
Development

No branches or pull requests

4 participants