Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safely copy paths to new *bo.Options #672

Merged
merged 3 commits into from
Dec 2, 2023

Conversation

jnichols-git
Copy link
Member

The SetDefaults function now copies over custom paths to the returned options by iterating the "old" routes (after they've had their own SetDefaults called) and setting the value for each key to a clone of the "old" value.

This was brought to my attention in issue #671, and my testing indicates that it resolves that issue; however, due to issues with the posted config (happens with copying over sometimes), my test wasn't an exact replica, so I will wait to hear from the issue creator. #655 was mentioned as a possible cause, and resolving that was what lead to this solution.

Closes #655.

@jnichols-git jnichols-git added bug Something isn't working 2.0 release Feature/Fix will be available in 2.0 Release labels May 24, 2023
@jnichols-git jnichols-git requested a review from jranson May 24, 2023 02:16
@coveralls
Copy link

coveralls commented May 24, 2023

Pull Request Test Coverage Report for Build 5083532234

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 92.762%

Totals Coverage Status
Change from base Build 5008571097: 0.001%
Covered Lines: 17185
Relevant Lines: 18526

💛 - Coveralls

@morningstart-liu
Copy link

Something is missing,in paths's SetDefaults() have a line : p.Custom = make([]string, 0) . take memory for Custom. your modify will lost Custom.

@jnichols-git
Copy link
Member Author

I'll have to get back to you on that specific chunk, I need to check with the original author to see what's meant to happen there.

@jnichols-git
Copy link
Member Author

Something is missing,in paths's SetDefaults() have a line : p.Custom = make([]string, 0) . take memory for Custom. your modify will lost Custom.

I meant to get back to you on this earlier, sorry. Slice size is automatically updated when you append to them, so the issue isn't the allocation here. We've discovered that the issue is (probably) that configs are reaching Merge before Custom is assigned to by SetDefault. That's why you're seeing empty Custom fields when Merge happens sometimes. Working on a fix!

@jranson jranson merged commit 6eb6269 into trickstercache:main Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 release Feature/Fix will be available in 2.0 Release bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom paths are not appended to config
4 participants