No "global" exec runner instance inside cloudfoundry pkg #1771
Conversation
marcusholl
force-pushed
the
pr/noGlobalExecRunnerInstanceInCloudfoundryPkg
branch
from
95d28cb to
0de2c3d
Compare
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| ) | ||
|
|
||
| //ReadServiceKeyAbapEnvironment from Cloud Foundry and returns it. | ||
| //Depending on user/developer requirements if he wants to perform further Cloud Foundry actions the cfLogoutOption parameters gives the option to logout after reading ABAP communication arrangement or not. | ||
| func ReadServiceKeyAbapEnvironment(options ServiceKeyOptions, cfLogoutOption bool) (ServiceKey, error) { | ||
| func (cf *CFUtils) ReadServiceKeyAbapEnvironment(options ServiceKeyOptions, cfLogoutOption bool) (ServiceKey, error) { |
There was a problem hiding this comment.
Method CFUtils.ReadServiceKeyAbapEnvironment has 51 lines of code (exceeds 50 allowed). Consider refactoring.
| @@ -26,11 +30,11 @@ func LoginCheck(options LoginOptions) (bool, error) { | |||
| var cfCheckLoginScript = append([]string{"api", options.CfAPIEndpoint}, options.CfAPIOpts...) | |||
|
|
|||
| var cfLoginBytes bytes.Buffer | |||
| c.Stdout(&cfLoginBytes) | |||
| _c.Stdout(&cfLoginBytes) | |||
There was a problem hiding this comment.
Does this make stdout inmutable for LoginCheck function?
There was a problem hiding this comment.
No. I already spottet that too. We have to reset the old stdout afterwards. But at the moment it is not possible to retrieve the current stdout via our wrapper. Needs to be added. At least the underlying cmd allow to get the currently set stream(s).
The point is: we have to ensure that the "cmdRunner" is not permanently changed inside the func. I had this already. As a result logging output stopped after calling login. Of course also for the subsequent cf deploy.
There was a problem hiding this comment.
I would propose to finalize on discussion in #1760 . This probably solves this whole problem and we can leave the whole function out
marcusholl
force-pushed
the
pr/noGlobalExecRunnerInstanceInCloudfoundryPkg
branch
2 times, most recently
from
be30204 to
91a9018
Compare
…package o Up to now we work on a private and shared instance of command.Command inside the cloudfounrdy package. We need to be able either configure this instance (environment variables) according to the use case. One option is to hand over an already configured instance which is used elsewhere. This is what we do with this commit. o With this commit we remove the instance which is shared within the cloudfounrdy package and to provide an instance with a receiver which gets handed over to the functions. Hence we are thread save: parallel invoctation of e.g. Login will not affect each other.
marcusholl
force-pushed
the
pr/noGlobalExecRunnerInstanceInCloudfoundryPkg
branch
from
91a9018 to
e24d442
Compare
|
@dominiklendle @DanielMieg @fwilhe; the behaviour is: we can provide an instance of Do you have concerns wrt this default behaviour? Other option would be to fail in case there is no |
|
/it |
|
Reality check performed by a merge commit between this PR and the cloudFoundryDeploy PR --> this works as expected. |
|
/it |
We have to be able to provide the cloudfoundry instance from outside to the cloudfoundry package in order to eg. perform a login. Reason: There are some environment variables which controls the behaviour of the cf cli (e.g.
CF_TRACE,CF_HOME).When we work upon a shared instance for all our tasks we have these issues:
1.) when the instance is global we have to ensure we perform a cleanup each time we are done with a task (e.g. resetting environment variables, streams (
stdout,stderr)2.) we cannot use that global instance in a parallel way. Currently we don't use that instance in a parallel mode, but in principle concurrency is well supported in go and we should be prepared for this.