Closed DanielBMann9000 closed 5 years ago
Thanks @DanielBMann9000 I will review this over the weekend
@giuliov Has this been reviewed? I'd prefer not to deploy a fork for my client.
At the moment we're working on pushing out the latest server version of the aggregator. That's our primary goal at the moment.
Your patch looks good. I suspect that we'll work on releasing the new webhooks version targeting the updated tfsaggregator core soon and we can then get this working too.
To expedite the PR, I'd you're willing to help, you could update the tfsaggregator-webhooks to use the latest aggregator-core/develop.
On 19 Mar 2018 14:38, "DanielBMann9000" notifications@github.com wrote:
@giuliov https://github.com/giuliov Has this been reviewed? I'd prefer not to deploy a fork for my client.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tfsaggregator/tfsaggregator-webhooks/pull/21#issuecomment-374214273, or mute the thread https://github.com/notifications/unsubscribe-auth/AD-uS2hQW1uzSzAFxwjWq5NyawZateR_ks5tf7TmgaJpZM4Sqd50 .
Sorry I lost track of this
I implemented a similar change in the develop
branch @DanielBMann9000
would you mind to take a loot at that?
@giuliov Doesn't my change preserve backwards compatibility with older versions, which your change doesn't appear to?
I check if the resourceContainers
element is present first, and fall back to the existing (presumably correct-for-older-versions) behavior if it isn't. If it is present, I use the value contained in there instead.
…D, causing authentication to fail with a 404 error