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

Refactor configuration #2300

Merged
merged 8 commits into from
Jun 8, 2020
Merged

Conversation

Inok
Copy link
Contributor

@Inok Inok commented May 31, 2020

It's a first step towards configuration refactoring.
All tests pass and I did my best to not introduce any breaking changes. However, the changes are quite huge, so it's a draft at this moment and I personally think it should not be released as part of 5.3.x. My intention in this PR is to show what I've done, to ask whether the changes are appropriate or not, and to discuss further steps.

@asbjornu, could you please review the changes? What do you think about them and about the next step I suggested below?

Description

I reversed the order of configuration merging. The previous approach was "read custom configuration and add default values to what's empty". The new approach is more natural: "create default configuration, override by configuration from source 1, override by...".

Previously, several config-level settings has been written to the branch config right on merging (IncrementStrategy and VersioningMode). The new approach does that on the very last step: on the configuration finalization. Now the configuration should be built like this:

var config = DefaultConfigurationProvider.GetDefaultConfiguration().Apply(new Config { NextVersion = "2.0.0" }).FinalizeConfig();

Next step

I guess, the next step is to do something like that:

Config config = new ConfigurationBuilder()
                              .AddDefault()
                              .AddYamlFile(...)
                              .AddLayer(new ConfigLayer { NextVersion = ... })
                              .Build();

So it will be similar to .NET Core's configuration.

@asbjornu
Copy link
Member

asbjornu commented Jun 2, 2020

This is great! Everything looks good so far. Just mark this as ready whenever you are. The proposed .NET Core-compatible configuration syntax is 💯.

@Inok
Copy link
Contributor Author

Inok commented Jun 4, 2020

The future improvements in this area seems quite complicated and I'd prefer to keep the current changes in a separate PR. So I'm marking this PR as ready for review. If the changes look good to merge at this point, I think it would be better to do that.

@Inok Inok marked this pull request as ready for review June 4, 2020 20:10
Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

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

Besides DefaultConfigProvider.CreateDefaultConfig being static I think this is fantastic work! 👏

src/GitVersionCore/Configuration/DefaultConfigProvider.cs Outdated Show resolved Hide resolved
@Inok
Copy link
Contributor Author

Inok commented Jun 8, 2020

I'm sorry, but I don't have enough time right now to do exactly what you've asked (immutable IConfig). So if the current changes look good enough to be merged - let's do that. I'll keep in mind that immutable IConfig, and I'll hopefully open a new PR with the IConfig when I have more time.

@Inok Inok requested a review from asbjornu June 8, 2020 07:54
Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

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

Fabulous work! 👏

@asbjornu asbjornu merged commit 4c945f4 into GitTools:master Jun 8, 2020
@asbjornu
Copy link
Member

asbjornu commented Jun 8, 2020

Thank you so much for this, @Inok! 🙏 Whenever you find time, I would love to accept a PR with immutable IConfig. 👍

@arturcic arturcic added this to the 5.3.x milestone Jun 9, 2020
@github-actions
Copy link

🎉 This issue has been resolved in version 5.3.6 🎉
The release is available on:

Your GitReleaseManager bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants