Ensure AppService Deployment Targets' health check respects proxy settings #35
Conversation
Fixes OctopusDeploy/Issues#7544 Updates the HealthCheck to use the new version of the Azure SDKs. It's in beta, so not touching the actual deployment code at this stage.
| /// are modifying the same shared state (eg Environment variables) across threads. | ||
| /// </remarks> | ||
| /// <typeparam name="TValue"></typeparam> | ||
| private class AutoResetMemento<TValue> : IDisposable |
There was a problem hiding this comment.
This looks like an implementation of "setup - test - teardown" pattern, how does this compare with just using [Setup] and [Teardown] of the test framework?
There was a problem hiding this comment.
Yeah you're probably right, the code is cute but it's probably unnecessarily complex given the scope of what actually needs to happen here. I'll pare it back.
There was a problem hiding this comment.
As I went to change it, I realised the reason I went for this in the first place: it allows a clean way, for a single test, to set things up and be sure that they will be restored to their original state, every time.
Setup and Teardown from the test framework apply to all tests in the fixture, so in order to achieve the same effect, I'll need to pull this test out to a separate fixture. I'll still do it... but there was some reasoning behind the implementation apart from just writing clever code ;)
There was a problem hiding this comment.
Thanks for the background~ Yep I can see why these infrastructures can be useful in enabling parameterised setups. Pulling the tests out in separate fixture may result in some repetitions in code, but that would be a little easier to understand for readers :)
There was a problem hiding this comment.
Just pushed the changes.
|
Hey Matt, just a note that "Relates to" is not one of the keywords supported by GitHub, so I'm not sure if it can auto close the related issue. If this PR addressed the issue entirely, perhaps we can use |
In this case, I did this intentionally. For Sashimi changes, the issue isn't actually fixed until we release the change in Server, so I intend to hold off and use the "Fixes" comment in my Server PR. But thanks for thinking about it :) |
…tings (#35) * Ensure HealthCheck respects configured proxy settings * Extract integration test base class to its own file * Pull common variable setup down to base class * Add test to ensure Health Check respects proxy settings * Ensure Proxy test works on both CI and locally * Fixes another small difference in testing between CI and local * More tweaks to test for cross-OS assertion * Fix up refactor in AppServiceBehaviourFixture * Simplify Proxy test code
Relates to OctopusDeploy/Issues#7544
The fluent version of the Azure SDK libraries have a bug where for certain operations, the explicitly set Proxy configuration is ignored. This causes Health Checks to fail when run in environments requiring a proxy server to contact Azure.
That version of the Azure SDK libraries is no longer being maintained.
This PR rewrites the Health Check to use the new (albeit still in beta) version of the Azure SDKs, which will be the One True Library going forward.
Testing