Skip to content

Conversation

@schickst
Copy link

@schickst schickst commented Feb 3, 2025

This pull request tries to fix [ISSUE]: Overrideconfig for strategies returns 'unknown key' error. Closes #4329.

Description

There are multiple issues when using /overrideconfig with the 'strategies' property.

First:
The overrideconfig uses a different format that the normal config. And they are then fed differently into the deserializer.

For the override config the following format seems to be used:
strategies=["Mainline","None"]

For the normal config the following format is used:
strategies:
- Mainline
- None

The second issue is that a cast from string[] to VersionStrategies[] has to happen.

My approach was to implement a custom type converter and registered it with the deserializer. The type converter then detects the config format and parses it accordingly.

In my tests it worked for both configuration sources. I am unsure if the location where I put the converter is the right place. I would prefer to avoid YamlDotNet as a direct dependency.

Also I hope that there is a simpler solution and avoid the converter all-together. Maybe this splitting behavior of the value beforehand can be improved.

Related Issue

#4329

Motivation and Context

How Has This Been Tested?

A test case has been added for the related overrideconfig strategies property. There are also many other test cases that might break, if the serialization/deserialization is wrong.

Checklist:

  • My code follows the code style of this project. (I hope so)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@schickst schickst requested a review from arturcic February 4, 2025 08:59
@arturcic arturcic requested a review from Copilot February 4, 2025 23:01
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

@arturcic arturcic enabled auto-merge (rebase) February 4, 2025 23:19
Copy link
Member

@arturcic arturcic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing this

@arturcic arturcic merged commit 3e74e07 into GitTools:main Feb 4, 2025
75 of 79 checks passed
@mergify
Copy link
Contributor

mergify bot commented Feb 4, 2025

Thank you @schickst for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ISSUE]: Overrideconfig for strategies returns 'unknown key' error

2 participants