Return the rc's based on config varible distill_on_error #568

samvarankashyap · 2018-05-02T17:23:56Z

Returns 0 even if one target provision is successfull when
distill_on_error = True
Returns rc > 0 even if one target provision fails when
distill_on_error = False

p3ck

LGTM

p3ck · 2018-05-02T17:28:35Z

linchpin/cli/__init__.py

-                pass
-
+            except Exception as e:
+                self.ctx.log_state('Error: {0}'.format(e))


Thanks you! ^^

I added this in my PR already, however...that's going to cause a conflict there.

https://github.com/CentOS-PaaS-SIG/linchpin/pull/565/files#diff-ff165001cb4def9d04e42540d89f083dR259

herlo · 2018-05-02T17:30:56Z

linchpin/shell/__init__.py

@@ -91,12 +89,17 @@ def _handle_results(ctx, results, return_code):

    # PRINT OUTPUT RESULTS HERE
    ctx.log_state(output)
+    distil_on_error = json.loads(lpcli.get_cfg('lp',


distill has two l's, probably ought to fix that so it is a proper spelling.

adl-bot · 2018-05-02T17:51:01Z

ci-linchpin-messageBus-trigger build #54: FAILURE: https://jenkins-continuous-infra.cloud.rhel8.upshift.engineering.redhat.com/job/ci-linchpin-messageBus-trigger/54/

herlo

I think there might have been some misunderstanding here. The 'distill_on_error' flag in linchpin.constants is specific for allowing the distiller to push out data to the linchpin.distilled file even if the return_code is not 0.

However, I think the purpose of this fix is to have a separate flag that allows a return_code of 0 if any target is successful. Thus, having a flag called 'return_success_from_targets' or some such would be more applicable here.

herlo · 2018-05-02T19:32:37Z

linchpin/shell/__init__.py

    use_actual_rcs = True
-    if use_actual_rcs:
+    if use_actual_rcs and not(distill_on_error):


I see you do have 'use_actual_rcs', but that should come from the linchpin.constants (or overridden by the linchpin.conf) and set to a default. Not hard coded here. Additionally, distill_on_error is a separate function and should occur if and only if that flag is set, it shouldn't be dependent on whether 'use_actual_rcs' is set. This would mean that one is dependent on the other to be true for certain conditions.

Returns 0 even if one target provision is successfull when distill_on_error = True Returns rc > 0 even if one target provision fails when distill_on_error = False

samvarankashyap · 2018-05-03T13:23:54Z

@herlo changes made!

adl-bot · 2018-05-03T13:24:10Z

ci-linchpin-messageBus-trigger build #61: FAILURE: https://jenkins-continuous-infra.cloud.rhel8.upshift.engineering.redhat.com/job/ci-linchpin-messageBus-trigger/61/

adl-bot · 2018-05-03T13:29:17Z

ci-linchpin-messageBus-trigger build #62: SUCCESS: https://jenkins-continuous-infra.cloud.rhel8.upshift.engineering.redhat.com/job/ci-linchpin-messageBus-trigger/62/

herlo · 2018-05-03T14:02:50Z

linchpin/shell/__init__.py

@@ -91,10 +89,12 @@ def _handle_results(ctx, results, return_code):

    # PRINT OUTPUT RESULTS HERE
    ctx.log_state(output)
+    use_actual_rcs = json.loads(lpcli.get_cfg('lp',
+                                              'use_actual_rcs').lower())


If you are using True/False, the proper use of this is to use ast.literaleval(value.title()), https://github.com/CentOS-PaaS-SIG/linchpin/blob/develop/linchpin/cli/__init__.py#L323 is a good example.

herlo · 2018-05-03T14:04:05Z

linchpin/linchpin.constants

@@ -26,6 +26,7 @@ external_providers_path = %(default_config_path)s/linchpin-x
 use_rundb_for_actions = False
 distill_data = False
 distill_on_error = False
+use_actual_rcs = False


This also needs a corresponding example and explanation in the https://github.com/CentOS-PaaS-SIG/linchpin/blob/develop/docs/source/examples/workspace/linchpin.conf

samvarankashyap · 2018-05-03T17:11:51Z

@herlo: changes made.

adl-bot · 2018-05-03T17:15:51Z

ci-linchpin-messageBus-trigger build #69: SUCCESS: https://jenkins-continuous-infra.cloud.rhel8.upshift.engineering.redhat.com/job/ci-linchpin-messageBus-trigger/69/

herlo

See comments.

herlo · 2018-05-03T17:13:45Z

docs/source/examples/workspace/linchpin.conf

+# if enabled True, even if one target provision is successfull linchpin 
+# returns exit code zero else returns the sum of all the return codes
+use_actual_rcs = False
+


Are you overriding what is in linchpin.constants here? It appears not, so it would be prudent to just comment it out.

herlo · 2018-05-03T17:17:16Z

linchpin/shell/__init__.py

@@ -91,10 +90,12 @@ def _handle_results(ctx, results, return_code):

    # PRINT OUTPUT RESULTS HERE
    ctx.log_state(output)
+    use_actual_rcs = ast.literal_eval(lpcli.get_cfg('lp',
+                                                    'use_actual_rcs'))



as a rule, you should add the .title() function to the item to get the 'True' or 'False' state before it's evaluated by the ast.literal_eval method. Something like this would be appropriate.

uar = lpcli.get_cfg('lp', 'use_actual_rcs')) use_actual_rcs = ast.literal_eval(uar.title())

In this way, if the value is 'true', or 'false', or any combination thereof, the value will get converted to proper title case, and return the proper boolean.

Okay , I was not aware of the rule . I tried ast.literal_eval(lpcli.get_cfg('lp','use_actual_rcs')) and it worked . Will make the change.

….literal_eval

herlo

LGTM

samvarankashyap · 2018-05-03T17:58:14Z

[merge]

adl-bot · 2018-05-04T02:23:51Z

ci-linchpin-messageBus-trigger build #71: FAILURE: https://jenkins-continuous-infra.cloud.rhel8.upshift.engineering.redhat.com/job/ci-linchpin-messageBus-trigger/71/

samvarankashyap requested review from herlo, p3ck and flowerncsu May 2, 2018 17:24

samvarankashyap added development cli configuration labels May 2, 2018

samvarankashyap self-assigned this May 2, 2018

p3ck approved these changes May 2, 2018

View reviewed changes

herlo reviewed May 2, 2018

View reviewed changes

samvarankashyap force-pushed the rc_distil_on_error branch from 1a2d77d to 63eb2da Compare May 2, 2018 17:49

herlo suggested changes May 2, 2018

View reviewed changes

herlo reviewed May 2, 2018

View reviewed changes

herlo changed the title ~~Return the rc's based on config varible distil_on_error~~ Return the rc's based on config varible distill_on_error May 2, 2018

samvarankashyap force-pushed the rc_distil_on_error branch from 680a0e9 to b41fb39 Compare May 3, 2018 13:18

samvarankashyap added 2 commits May 3, 2018 09:22

Return the rc's based on config varible distil_on_error

17b1582

Returns 0 even if one target provision is successfull when distill_on_error = True Returns rc > 0 even if one target provision fails when distill_on_error = False

Use use_actual_rcs confifguration parameter instead of distill_on_error

55f8953

samvarankashyap force-pushed the rc_distil_on_error branch from b41fb39 to 55f8953 Compare May 3, 2018 13:23

herlo suggested changes May 3, 2018

View reviewed changes

Added documentation for use_actual_rcs & replaced json.loads with ast…

cc60cb0

….literal_eval

samvarankashyap force-pushed the rc_distil_on_error branch from 13ac023 to cc60cb0 Compare May 3, 2018 17:53

herlo approved these changes May 3, 2018

View reviewed changes

adl-bot merged commit 2761bc2 into CentOS-PaaS-SIG:develop May 3, 2018

adl-bot mentioned this pull request Nov 19, 2018

Add variables in inventory generation #703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return the rc's based on config varible distill_on_error #568

Return the rc's based on config varible distill_on_error #568

samvarankashyap commented May 2, 2018

p3ck left a comment

p3ck May 2, 2018

herlo May 2, 2018

herlo May 2, 2018

herlo May 2, 2018

adl-bot commented May 2, 2018

herlo left a comment •

edited

Loading

herlo May 2, 2018

samvarankashyap commented May 3, 2018

adl-bot commented May 3, 2018

adl-bot commented May 3, 2018

herlo May 3, 2018

herlo May 3, 2018

samvarankashyap commented May 3, 2018

adl-bot commented May 3, 2018

herlo left a comment

herlo May 3, 2018

herlo May 3, 2018

samvarankashyap May 3, 2018

herlo left a comment

samvarankashyap commented May 3, 2018

adl-bot commented May 4, 2018

Return the rc's based on config varible distill_on_error #568

Return the rc's based on config varible distill_on_error #568

Conversation

samvarankashyap commented May 2, 2018

p3ck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adl-bot commented May 2, 2018

herlo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samvarankashyap commented May 3, 2018

adl-bot commented May 3, 2018

adl-bot commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samvarankashyap commented May 3, 2018

adl-bot commented May 3, 2018

herlo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herlo left a comment

Choose a reason for hiding this comment

samvarankashyap commented May 3, 2018

adl-bot commented May 4, 2018

herlo left a comment •

edited

Loading