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

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Contributor

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.

@nlewo nlewo force-pushed the nlewo:jobset-error branch from 453488e to 12271a7 Aug 26, 2019
@@ -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

This comment has been minimized.

Copy link
@grahamc

grahamc Aug 26, 2019

Member

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.

This comment has been minimized.

Copy link
@nlewo

nlewo Aug 26, 2019

Author Member

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.
@nlewo nlewo force-pushed the nlewo:jobset-error branch from 12271a7 to c8983ca Aug 26, 2019
@grahamc grahamc merged commit 906d249 into NixOS:master Aug 26, 2019
gilligan added a commit to nlewo/hydra-cli that referenced this pull request Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.