search

cloudfoundry / cf-deployment-concourse-tasks

Apache License 2.0
23 stars 76 forks source link

shared-functions: Add BOSH_DEPLOY_ARGS to uptimer_config #97

Closed diogomatsubara closed 5 years ago

diogomatsubara commented 5 years ago

What is this change about?

Describe the change and why it's needed. This change makes the bosh_deploy function consistent when the DEPLOY_WITH_UPTIME_MEASUREMENTS is used. Previously if you passed BOSH_DEPLOY_ARGS in your task configuration without using DEPLOY_WITH_UPTIME_MEASUREMENTS, bosh deploy would respect your BOSH_DEPLOY_ARGS. If DEPLOY_WITH_UPTIME_MEASUREMENTS is set to true, then BOSH_DEPLOY_ARGS would be silently ignored.

Please provide contextual information.

N/A, we found this issue in our internal concourse pipeline and thought that would be nice to make things consistent.

Please check all that apply for this PR:

Did you update the README as appropriate for this change?

How should this change be described in release notes?

Consistently pass BOSH_DEPLOY_ARGS to bosh_deploy() shared function.

What is the level of urgency for publishing this change?

Tag your pair, your PM, and/or team!

It's helpful to tag a few other folks on your team or your team alias in case we need to follow up later.

cfdreddbot commented 5 years ago

:white_check_mark: Hey diogomatsubara! The commit authors and yourself have already signed the CLA.

cf-gitbot commented 5 years ago

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/167999708

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

davewalter commented 5 years ago

Hi @diogomatsubara,

This looks like this change might not work if multiple args are specified. The BOSH_DEPLOY_ARGS parameter is handled differently by the bosh-deploy task in the case that uptimer is not being used. Have you tested this scenario? It looks like it might be possible to use the jq --args flag to separate out each flag when appending them to the command_args array in the uptimer config file, but we haven't fully explored this option yet. The other problem is that we aren't using this parameter in our own pipelines, so it's hard for us to have confidence in a change without a lot of manual testing.

Let us know how you want to proceed.

Regards, Dave and @vitreuz

diogomatsubara commented 5 years ago

Hi @davewalter, thanks for the quick reply and the initial review!

IIUC, in case the uptimer tool is not being used, the $BOSH_DEPLOY_ARGS will expand (via ${@}) here: https://github.com/cloudfoundry/cf-deployment-concourse-tasks/blob/master/shared-functions#L249 as individual arguments to the deploy action, while when the uptimer tools is being used, then all the extra args passed by in BOSH_DEPLOY_ARGS will be added to the command_args in the uptimer config as a single argument.

In our pipeline, where we use the uptimer tool, this patch correctly passed the arguments to bosh deploy as we expected but you're right that we treat that as single argument rather than as individual arguments passed to bosh deploy. So, to clarify and get this in shape to be merged, you want this to be changed so that individual arguments are treated as individual arguments for BOSH_DEPLOY_ARGS or would be ok to change ${@} expansion to use ${BOSH_DEPLOY_ARGS} instead?

I'm happy to test this and make sure my changes won't break existing behaviour. What would you like to see in terms of testing for this?

acosta11 commented 5 years ago

Hi @diogomatsubara ,

We went ahead and made the proposed change in https://github.com/cloudfoundry/cf-deployment-concourse-tasks/commit/aa52b1ca4b03f6c2c883a9c22bded0637daf3d2f . We'll probably just do a sanity check by running this in our pipelines before cutting an official release. If you would like to start consuming it before then, it's technically available directly on master once we release an updated docker image with the jq version bump we depend on. If there's something we missed here, feel free to open a new pr.

Thanks, Andrew and @davewalter