ansibleplaybookbundle / apb-base

Base image for APB development
Apache License 2.0
8 stars 14 forks source link

Bogus shift usage in entrypoint #49

Closed rgolangh closed 5 years ago

rgolangh commented 6 years ago

The entrypoint assumes a second ($2) argument is a json string but immediatly after evaluating the first one if shifts, rendering the $2 empty.

Motivation New APB that adapts to the ansible-runner invocation and passes in arguments should use this

Modification Remove the useless 'shift' usage

Result Invoking the entrypoint now works again:

entrypoint.sh provision '{"key":"value"}'
rgolangh commented 6 years ago

@jmrodri @mhrivnak please review

mhrivnak commented 6 years ago

The way an APB gets invoked is documented here: https://github.com/openshift/ansible-service-broker/blob/ansible-service-broker-1.4.1-1/docs/service-bundle.md#input

Of note, there are three arguments. As-is, entrypoint.sh grabs the first argument into a variable called ACTION and then does the shift, leaving two remaining arguments.

The change you've proposed here would break existing APBs. That said, replacing the shift with different logic could make sense, or changing the arguments that an APB takes could also make sense (I never liked that it takes --extra-vars as its second argument).

But, it would help to take a step back and describe what it is you're trying to accomplish. Could you elaborate on your "Motivation" section? Is there an ansible-runner format you're trying to conform to? Is there a concrete use case you can describe in detail that works without shift but fails with it?

rgolangh commented 6 years ago

I don't see any usage of the 2nd argument, that is why I thought to remove it. But I didn't look into the service-bundle.md yet so I could be missing something.

My usage is using my apb container with or without the broker (manually invoking the container and passing in the 'action' and then 'args')

jmrodri commented 6 years ago

@rgolangh It looks like the entrypoint.sh was rewritten during this commit which probably removed the need for the shift https://github.com/ansibleplaybookbundle/apb-base/commit/4c7e68dd719754c2c7df7b6c023d9e1f77d4effc prior versions after taking $1, they would use $@. It is true that the second argument isn't being used right now. But I believe older brokers will pass in --extra-vars.

rgolangh commented 6 years ago

@jmrodri @mhrivnak I'll reiterate my patch, which will obviously broke other invocations

djzager commented 5 years ago

@mhrivnak + @jmrodri do you have any thoughts on this? Seems like a reasonable update.

jmrodri commented 5 years ago

@djzager it's fine with me.