#2084 Apply default config file paths in `GetMergedOcelotJson` when providing the `folder` argument of `AddOcelot` by ben-bartholomew · Pull Request #2120 · ThreeMammals/Ocelot

ben-bartholomew · 2024-07-12T14:51:26Z

Fixes #2084

After upgrade to Ocelot 23.2, ocelot.global.json file no longer gets merged correctly #2084

Proposed Changes

Combining user provided folder path with the default names for global, primary, or environment config files, if those filenames are not provided using folder configuration with AddOcelot()
- This is only done when the default names are used (i.e., when the user does not specify a filename)
Added unit test for this issue

raman-m · 2024-07-12T15:27:24Z

Hello Ben!
Thank you for opening the PR!
I'll review today and possibly merge tomorrow.

src/Ocelot/DependencyInjection/ConfigurationBuilderExtensions.cs

test/Ocelot.UnitTests/DependencyInjection/ConfigurationBuilderExtensionsTests.cs

raman-m · 2024-07-16T10:02:35Z

@ggnaegi Thanks for approving this, but there is no acceptance test(s) for the linked bug issue.
I'm working on acceptance test now...

raman-m

Ready for delivery❗

raman-m · 2024-07-16T15:42:14Z

src/Ocelot/DependencyInjection/ConfigurationBuilderExtensions.cs

            string primaryConfigFile = null, string globalConfigFile = null, string environmentConfigFile = null, bool? optional = null, bool? reloadOnChange = null) // optional injections
        {
            var json = GetMergedOcelotJson(folder, env, null, primaryConfigFile, globalConfigFile, environmentConfigFile);
+            primaryConfigFile ??= Path.Join(folder, PrimaryConfigFile); // if not specified, merge & write back to the same folder


Hi Ben,
We overlooked this change, and acceptance testing has revealed the issue. The folder information should be retained, and the file must be written back to the same folder.

raman-m · 2024-07-16T15:54:32Z

Thank you, @ben-bartholomew, for your contribution!

@raman-m

…Blue Olympic Balumbes release * #2084 Apply default config file paths in `GetMergedOcelotJson` when providing the `folder` argument of `AddOcelot` (#2120) * Adding unit test first * Fixing default global config file not being found in folder * Adding PR trait to test * Backing out whitespace changes * Code review by @raman-m * Create Configuration feature folder and move test classes * Adjust namespace and review what we have * Acceptance tests for #2084 user scenario --------- Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com> * Bump Steeltoe.Discovery.Eureka from 3.2.5 to 3.2.8 in /src/Ocelot.Provider.Eureka (#2122) * Bump Steeltoe.Discovery.Eureka in /src/Ocelot.Provider.Eureka Bumps [Steeltoe.Discovery.Eureka](https://github.com/SteeltoeOSS/Steeltoe) from 3.2.5 to 3.2.8. - [Release notes](https://github.com/SteeltoeOSS/Steeltoe/releases) - [Changelog](https://github.com/SteeltoeOSS/Steeltoe/blob/main/Steeltoe.Release.ruleset) - [Commits](SteeltoeOSS/Steeltoe@3.2.5...3.2.8) --- updated-dependencies: - dependency-name: Steeltoe.Discovery.Eureka dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> * Bump Steeltoe.Discovery.ClientCore from 3.2.5 to 3.2.8 --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com> * #2110 Review load balancing and independent fetching the list of services in `Kube` provider (#2111) * Move the creation of the services list from the class field to the method, to prevent modification list from different threads * Early return after data checking * Add unit test for concurrent get list of services * Add logging for invalid service configuration error in RoundRobin load balancer * Code review by @raman-m * Workaround for mistakes made during acceptance testing of load balancing versus service discovery, where tests designed for parallel requests were mistakenly executed sequentially. This resulted in load balancers being loaded by sequential `HttpClient` calls, which was a significant oversight. * Let's DRY StickySessionsTests * Add acceptance tests, but... RoundRobin is not actually RoundRobin 😁 -> 😆 * Independent static indexing iterators per route via service names * Stabilize `CookieStickySessions` load balancer. Review tests after refactoring of `RoundRobin` load balancer * Refactor Lease operation for load balancing. Review LeastConnection load balancer * Leasing mechanism in Round Robin load balancer * Acceptance tests, final version * Apply Retry pattern for K8s endpoint integration * Fix IDE warnings and messages * Follow suggestions and fix issues from code review by @ggnaegi * Bump KubeClient from 2.4.10 to 2.5.8 * Fix warnings * Final version of `Retry` pattern --------- Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com> * Downgrade the Warning to Information on missing `Content-Length` header in `MultiplexingMiddleware` (#2146) * fix: downgrade the warning to information on missing content-length header * chore: add route name to logs * test: fixing multiplexing middleware tests * Code review by @raman-m --------- Co-authored-by: Paul Roy <paul.roy@astriis.com> Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com> * Correct the broken link to the GraphQL sample's `README.md` (#2149) Signed-off-by: Emmanuel Ferdman <emmanuelferdman@gmail.com> Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com> * #2116 Escaping unsafe pattern values of `Regex` constructor derived from URL query parameter values containing special `Regex` chars (#2150) * regex escape handling for url templates * refactored regex method to lamda version * Quick code review by @raman-m * added acceptance test for url regex bug * moved acceptance test to routing tests * Convert to theory: define 2 test cases --------- Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com> * #2119 Review load balancing (2nd round) and redesign `DefaultConsulServiceBuilder` with `ConsulProviderFactory` refactoring to make it thread safe and friendly (#2151) * Review tests * History of Service Discovery testing: add traits * LoadBalancer traits * #2119 Steps to Reproduce * Reuse service handlers of `ConcurrentSteps` * Reuse service counters of `ConcurrentSteps` * Add LoadBalancer namespace and move classes * Move `Lease` * Move `LeaseEventArgs` * Analyze load balancers aka `ILoadBalancerAnalyzer` interface objects * Prefer using named local methods as delegates over anonymous methods for awesome call stack, ensuring the delegate's typed result matches the typed balancer's creator. Additionally, employ an IServiceProvider workaround. * Review load balancing. Assert service & leasing counters as concurrent step. Final version of acceptance test. * Fixed naming violation for asynchronous methods: `Lease` -> `LeaseAsync` * Fix ugly reflection issue of dymanic detection in favor of static type property * Propagate the `ConsulRegistryConfiguration` object through `HttpContext` in the scoped version of the default service builder, utilizing the injected `IHttpContextAccessor` object. Update `ConsulProviderFactory`. Update docs. Update tests. * Add tests from clean experiment * Final review of the tests * Review `IHttpContextAccessor` logic. Convert anonymous delegates to named ones in placeholders processing * Tried to enhance more, but failed --------- Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Emmanuel Ferdman <emmanuelferdman@gmail.com> Co-authored-by: Ben Bartholomew <70723971+ben-bartholomew@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Roman <61905975+antikorol@users.noreply.github.com> Co-authored-by: Paul Roy <paul.achess.roy@gmail.com> Co-authored-by: Paul Roy <paul.roy@astriis.com> Co-authored-by: Emmanuel Ferdman <emmanuelferdman@gmail.com> Co-authored-by: Finn <26823828+int0x81@users.noreply.github.com>

ben-bartholomew added 3 commits July 12, 2024 10:39

Adding unit test first

315b952

Fixing default global config file not being found in folder

60cd2d4

Adding PR trait to test

8b0df64

raman-m assigned ben-bartholomew Jul 12, 2024

raman-m added bug Identified as a potential bug Configuration Ocelot feature: Configuration Oct'24 October 2024 release labels Jul 12, 2024

raman-m added this to the v23.3.x Hotfixes milestone Jul 12, 2024

raman-m requested review from RaynaldM, ggnaegi and raman-m July 12, 2024 15:32

raman-m reviewed Jul 14, 2024

View reviewed changes

ben-bartholomew and others added 4 commits July 15, 2024 09:09

Backing out whitespace changes

4c9f205

Code review by @raman-m

92fe482

Create Configuration feature folder and move test classes

78053d6

Adjust namespace and review what we have

52051ec

ggnaegi approved these changes Jul 15, 2024

View reviewed changes

raman-m changed the title ~~Fixing default config file paths in GetMergedOcelotJson when providing a folder~~ #2084 Apply default config file paths in GetMergedOcelotJson when providing the folder argument of AddOcelot Jul 16, 2024

Acceptance tests for ThreeMammals#2084 user scenario

c1d8624

raman-m approved these changes Jul 16, 2024

View reviewed changes

raman-m merged commit d418b3f into ThreeMammals:develop Jul 16, 2024

raman-m removed the Oct'24 October 2024 release label Aug 12, 2024

raman-m mentioned this pull request Oct 3, 2024

Release 23.3.4 | v23.3 Hotfixes | Blue Olympic Balumbes release #2162

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

#2084 Apply default config file paths in `GetMergedOcelotJson` when providing the `folder` argument of `AddOcelot`#2120

#2084 Apply default config file paths in `GetMergedOcelotJson` when providing the `folder` argument of `AddOcelot`#2120
raman-m merged 8 commits intoThreeMammals:developfrom
ben-bartholomew:develop

ben-bartholomew commented Jul 12, 2024 •

edited by raman-m

Loading

Uh oh!

raman-m commented Jul 12, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raman-m commented Jul 16, 2024

Uh oh!

raman-m left a comment

Uh oh!

raman-m Jul 16, 2024 •

edited

Loading

Uh oh!

raman-m commented Jul 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

ben-bartholomew commented Jul 12, 2024 • edited by raman-m Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes #2084

Proposed Changes

Uh oh!

raman-m commented Jul 12, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raman-m commented Jul 16, 2024

Uh oh!

raman-m left a comment

Choose a reason for hiding this comment

Ready for delivery❗

Uh oh!

raman-m Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raman-m commented Jul 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ben-bartholomew commented Jul 12, 2024 •

edited by raman-m

Loading

raman-m Jul 16, 2024 •

edited

Loading