Skip to content
This repository has been archived by the owner on Feb 11, 2020. It is now read-only.

More strict check for disable-errands task #310

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

antonsoroko
Copy link

Thanks for contributing to pcf-pipelines. To speed up the process of reviewing your pull request please provide us with:

  • A short explanation of the proposed change:
    Currently disable-errands task can disable errands which you don't expect it to disable
  • An explanation of the use cases your change solves:
    Try to disable errands in p-redis tile, e.g.
errands_to_disable: "register-broker,on-demand-broker-smoke-tests,upgrade-all-service-instances"

disable-errands task will also disable smoke-tests errand

  • Expected result after the change:
echo broker-registrar smoke-tests register-broker on-demand-broker-smoke-tests upgrade-all-service-instances broker-deregistrar delete-all-service-instances-and-deregister-broker | jq --arg to_disable 'register-broker
on-demand-broker-smoke-tests
upgrade-all-service-instances' --raw-input --raw-output 'split(" ")
    | reduce .[] as $errand ([];
       if $to_disable | split("\n") | index($errand) then
         . + [$errand]
       else
         .
       end)
    | join("\n")'
register-broker
on-demand-broker-smoke-tests
upgrade-all-service-instances

Note: I also tried to use regex for this but I faced issue with jq - jqlang/jq#1629

  • Current result before the change:
echo broker-registrar smoke-tests register-broker on-demand-broker-smoke-tests upgrade-all-service-instances broker-deregistrar delete-all-service-instances-and-deregister-broker | jq --arg to_disable 'register-broker
on-demand-broker-smoke-tests
upgrade-all-service-instances' --raw-input --raw-output 'split(" ")
    | reduce .[] as $errand ([];
       if $to_disable | contains($errand) then          
         . + [$errand]
       else
         .
       end)
    | join("\n")'
smoke-tests
register-broker
on-demand-broker-smoke-tests
upgrade-all-service-instances
  • Links to any other associated PRs or issues:

  • I have viewed signed and have submitted the Contributor License Agreement

  • I have made this pull request to the master branch

  • I have run all the unit tests

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

@abbyachau
Copy link
Contributor

Hi @antonsoroko thank you for creating this issue. Could you please clarify how you ran into this issue, which pipeline you used, did you make modifications to the pipeline, for example. We currently do not offer a pipeline to install service tiles, nor is there a feature in which you can disable errands for service tiles. The upgrade-tile pipeline (which does patch updates of PAS and service tiles, allow you to enable or disable errands - not to choose which ones specifically to disable). Thanks for helping us triage the issue. We also have the following pull request scheduled for engineering review: #287.

@antonsoroko
Copy link
Author

@abbyachau
Hi. We use upgrade-tile pipeline with on-fly modifications (yaml-patch).
And because of our customer needs (currently they disable on-demand service brokers) we need to be able to specify which errands should be disabled.
I do not see any harm in this PR (this is a simple bug fix).
Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants