Make the process fail if the changelog file is not found

[MangelMaxime]

Hello @baronfel,
this is an issue created from our discussion on F# slack.

Currently, if the changelog file is not found there is no indication for the user. The dotnet pack command succeed as if nothing was wrong.

I personally think that if the changelog file is not found at the specified path, the process should fail because it is generating something from an invalid context.

For example, when we specify the PackageLicenseFile node in an fsproj the task is failing if the provided value is invalid or the file is missing/not found.

[baronfel]

I think this is a good idea, but I think it should be a warning by default, mostly because the design of this library is 'implicit' by default. Let's dive into why.

When you add PackageLicenseFile to a project file, you had to take an explicit action in order to opt in to using that feature. That tells the MSBuild target author that you explicitly want to use the License File feature, so they can be quite strict.

This library by comparison currently has no such opt-in requirement - it implicitly discovers and uses a CHANGELOG file in the current project directory if one exists. This means that there's no user action we can point to to justify a fail-by-default behavior in my opinion.

There are two ways we could address this IMO:

• keep the implicit behavior, and add a warning when no changelog file could be found either at the default location or a user-specified location, or
• remove the implicit behavior, require the user to specify a ChangelogFile property, then default to an error if not found

[MangelMaxime]

Thank you for the clear explanations.

Personally, I always preferred explicit behaviour from my code and so would prefer the second option.

[baronfel]

I agree. A few other people have spoken up as well. We should remove the implicit mode and provide a helpful error if the user doesn't specify a ChangelogFile. That way there's no confusion about if something is happening or not.

[MangelMaxime]

Looking at the code it seems like there are already severals checks done for the file existence:

KeepAChangelog/src/Ionide.KeepAChangelog.Tasks/buildMultiTargeting/Ionide.KeepAChangelog.Tasks.targets

Lines 24 to 28 in 7ad0461

	<Target
	Name="GetChangelogVersion"
	Condition="'$(ChangelogFile)' != '' and Exists('$(ChangelogFile)')"
	Inputs="$(ChangelogFile)"
	Outputs="UnreleasedChangelog;CurrentReleaseChangelog;AllReleasedChangelogslLatestReleaseNotes">

KeepAChangelog/src/Ionide.KeepAChangelog.Tasks/Library.fs

Lines 49 to 54 in 7ad0461

	override this.Execute() : bool =
	let file = this.ChangelogFile |> FileInfo
	if not file.Exists then
	this.Log.LogError($"The file {file.FullName} could not be found.")
	false

I guess they are currently only generating warnings and we should hard fail in these cases. What would be the correct way of failing?

• In F#, we can probably throw an exception
• In MSBuild ?

[baronfel]

@tboby just added two msbuild warnings verifying that the changelog file is provided and exists.

[tboby]

So, now that it's no longer implicit, what behaviour do we want?

I suspect that the WarnOnNoChangelogFileExists (when the given file isn't found) should possibly be an error now.

Do we want the WarnOnNoChangelogFileSet to be an error as well, perhaps mentioning "Please set this property ... , or remove the Ionide.KeepAChangelog.Tasks package"?

Alternatively, do we want to add a property to supress the validation? Trying to think if it would be annoying if you Build.props this package and then your test project starts throwing errors.