Dynatrace / libbuildpack-dynatrace

Common resources used for Dynatrace integration in Cloudfoundry Buildpacks.
Apache License 2.0
1 stars 5 forks source link

Added oneagenturl variable to download agent via custom url #16

Closed manitheja1 closed 4 years ago

manitheja1 commented 4 years ago

This change introduces a new parameter "OneAgentURL" in the VCAP_SERVICES which can be used to configure custom location to download the agent

DTMad commented 4 years ago

Looks good to me! Could you do me a favor and rename the variable to something like "CustomOneAgentURL" so it's clearer that this is not a mandatory field with default configuration?

Thanks, Marco

manitheja1 commented 4 years ago

Thanks for reviewing Marco!! Will update the pull request accordingly.

Thanks, Manitheja

manitheja1 commented 4 years ago

Hi Marco, I have renamed the variable as you suggested. Please let me know if the changes are good!!

Thanks, Manitheja

manitheja1 commented 4 years ago

Any update? Thanks, Manitheja

arthfl commented 4 years ago

Ahoy @manitheja1,

thanks for renaming the variable, but can you please change the "outside" property to lowercase, so it fits in with the others. Meaning in line 215 of your PR, change

CustomOneAgentURL:   queryString("CustomOneAgentURL"),

to

CustomOneAgentURL:   queryString("customoneagenturl"),

And also this check here will need some adaptions as well: https://github.com/Dynatrace/libbuildpack-dynatrace/pull/16/files#diff-6824f41d082d8d1e355aae9838a40779R220 Otherwise the hook will exit as soon as it queries for the credentials.

manitheja1 commented 4 years ago

Hi Arthofer, I would do the change and update pr for the outside property . Meanwhile i would like to know more about the check you mentioned(https://github.com/Dynatrace/libbuildpack-dynatrace/pull/16/files#diff-6824f41d082d8d1e355aae9838a40779R220)

I am not sure if i understood the second part correctly. As this is optional parameter and also the check mentioned above doesn't have this variable so that it would not affect the logic if someone don't specify the value for "cistomoneagenturl". Could you please tell me why the hook will exit or what changes needs to be done for the check?

Thanks, Manitheja

arthfl commented 4 years ago

@manitheja1 The mentioned check tests if environment-id AND apitoken are set. And if not, it will cause a failure at https://github.com/Dynatrace/libbuildpack-dynatrace/blob/55c447f429cb064ae7a89e806c7c7109fc25f43e/hook.go#L68

So this needs to be handled for your case.

manitheja1 commented 4 years ago

Hi @arthfl ,

Yes you are right!! But it was not failing in our case(when tested) because we are providing environmentid and api token additinally along with customoneagenturl and hence its not breaking.

I will update the pr accordingly. Thanks for the clarification!!

Thanks, manitheja

arthfl commented 4 years ago

Yeah, if you provide those, the deployment will obviously not fail :-) But in this special case, somebody might not provide them, because they already downloaded the agent package previously and put it somewhere. Thus they might assume that those parameters are not needed anymore, which is true. So that case should be handled.

manitheja1 commented 4 years ago

Hi @arthfl ,

Thanks for suggesting the changes. I have made changes for both of the points you have mentioned above. Could you please review it and let me know if changes are good?

Thanks, Manitheja

arthfl commented 4 years ago

@manitheja1 Will do so and try to get back to you soon. Thanks for being so cooperative :-)

manitheja1 commented 4 years ago

Hi @arthfl ,

Any update on this please?

Thanks, Manitheja

arthfl commented 4 years ago

@manitheja1 Sorry for the delay. Things were a bit busy around here with vacations and stuff :-)

The PR looks good to us now. Will merge it.

manitheja1 commented 4 years ago

Hi Team, Thanks everyone for all the support in getting the changes merged!!

Thanks, Manitheja

manitheja1 commented 4 years ago

Hi Team,

Wanted to know when will the changes will be added to release? This will help us to request cloudfoundary team to include this latest hook release in non java buildpacks.

Thanks, Manitheja