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

Add error boolean attribute to jobset API response #673

Merged
merged 1 commit into from Aug 26, 2019

Conversation

nlewo
Copy link
Member

@nlewo nlewo commented Aug 20, 2019

This attribute allows to know if an error occurred or not. Note we can
not use the errormsg attribute because it can be arbitrarily long and
is excluded from the jobset API response.

@nlewo
Copy link
Member Author

nlewo commented Aug 20, 2019

In this PR, I use true and false JSON values while 1 and 0 are used in the rest of the API to represent boolean (see project.hidden attribute for instance). Maybe we should use integer values for homogeneity, even if boolean values are better. WDYT?

@gilligan
Copy link
Contributor

gilligan commented Aug 26, 2019

LGTM.

Maybe we should use integer values for homogeneity, even if boolean values are better. WDYT?

Personally i'd definitely prefer those values to be boolean values and i'd rather like to see the other occurrences of integers that should actually be booleans fixed.

Edit: Actually I think I would prefer successful instead of error in the response. I would argue that makes it more obvious that it's a boolean value.

@@ -87,7 +87,8 @@ sub jobsetToHash {
checkinterval => $jobset->checkinterval,
triggertime => $jobset->triggertime,
fetcherrormsg => $jobset->fetcherrormsg,
errortime => $jobset->errortime
errortime => $jobset->errortime,
successful => $jobset->errormsg eq "" ? JSON::true : JSON::false
Copy link
Member

Choose a reason for hiding this comment

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

Does this query always fetch the entire error message every time? Did it before?

Does errormsg mean stderr output?

If so, the presence of stderr output doesn't mean it failed which makes "successful" a misnomer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually the semantic of successful is not correct: when a fetch error occurs, we don't want to have a successful attribute set to true.

So, we propose has_errormsg attribute instead.

This attribute allows to know if an error occurred or not: when an
error occurs, errormsg is not an empty string. Note we can not use the
errormsg attribute because it can be arbitrarily long and is excluded
from the jobset API response.
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.

None yet

3 participants