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

Make sure custom configuration file path is returned when custom configuration is set #4483

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kucuk-furkan
Copy link

@kucuk-furkan kucuk-furkan commented Apr 2, 2025

Description

In this pr, /config argument usage controlled and issue when using it fixed.

  • Introduce a new test method ReturnConfigurationFilePathIfCustomConfigurationIsSet in ConfigurationFileLocatorTests.cs to
    verify custom config file handling.
  • Update GitVersion.Configuration.Tests.csproj to copy Configuration/CustomConfig.yaml to the output directory.
  • Enhance GetConfigurationFile method in ConfigurationFileLocator.cs to return the custom config file path if set and exists.
  • Add CustomConfig.yaml with various versioning settings.

Related Issue

Resolves #4480

Motivation and Context

This bugfix resolves #4480 issue. In earlier changes there is a change to how should tool locating configuration files. It cause a problem if /config arguments is used. Now if its used and configuration file is presents, locator returns custom configuration file path directly.

This isn't a problem before this change merged. In dotnet Path.Combine method handles if path is rooted automatically. But with this pr, tool stops using Path.Combine and starts directly looking under givin directory path. For further information, you can check this Microsoft documentation. In this documentation; we are interested with combine method's examples section - path1 and path3 combine example -.

How Has This Been Tested?

All existing tests pass.
New tests added.

Checklist:

  • My code follows the code style of this project.
  • 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.

Copy link
Contributor

@HHobeck HHobeck left a comment

Choose a reason for hiding this comment

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

I have reviewed your code changes.

@arturcic arturcic self-requested a review April 3, 2025 09:39
@kucuk-furkan kucuk-furkan requested a review from HHobeck April 4, 2025 05:39
@arturcic
Copy link
Member

arturcic commented Apr 4, 2025

@kucuk-furkan please check why the the tests fail on ubuntu
image
then I can review the code

@kucuk-furkan
Copy link
Author

@arturcic i didn't fix it on purpose to ask this. I shouldn't be the decider i think. I'm just a user of this tool and try to help fix my issue.

Tests fails because of this Also if i apply your suggestion, tool stops working as case insensitive. Case insensitive support was added couple weeks ago. I'm not sure this suggestion should be applied or not

Either i can remove those tests too or put back case insensivity support back. What should i do? Someone who responsible for this project tell me what to do. I think leaving managing case sensivity to OS is better choice. But someone opened this as an issue then fixed it.

Which direction should i go?

@arturcic
Copy link
Member

arturcic commented Apr 4, 2025

The case insensitive should be kept for the default file names, for the custom config file probably better to leave it to the OS

@arturcic
Copy link
Member

arturcic commented Apr 5, 2025

@kucuk-furkan please make sure you do rebase of your branch on top of main branch, instead or mergin main into your branch. We prefer to have clean history and it's clear where the PR branch started in the history

…iguration is set

Introduce a new test method `ReturnConfigurationFilePathIfCustomConfigurationIsSet` in `ConfigurationFileLocatorTests.cs` to verify custom config file handling. Update `GitVersion.Configuration.Tests.csproj` to copy `Configuration/CustomConfig.yaml` to the output directory. Enhance `GetConfigurationFile` method in `ConfigurationFileLocator.cs` to return the custom config file path if set and exists. Add `CustomConfig.yaml` with various versioning settings.
@kucuk-furkan
Copy link
Author

@arturcic i think its ready now. Tests looks fine. I rebased my fork and for cleaner look, i squashed my commits.

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]: 6.2.0 seems to be adding unnecessary pre-release suffix to the tag
3 participants