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

Suppress autostart warning #2776

Merged
merged 1 commit into from
Jun 10, 2019

Conversation

HebaruSan
Copy link
Member

Problem

If your auto start instance is not valid and you run ckan compare 1.2.3 4.5.6, you get an extra warning message in addition to the expected output:

342 [1] WARN CKAN.KSPManager (null) - Auto-start instance was invalid: Exception of type 'CKAN.InvalidKSPInstanceKraken' was thrown.
"1.2.3" is lower than "4.5.6".

This isn't useful because we're not trying to do anything with auto start instances, we're just comparing version strings.

The same message is printed by many of the tests.

Cause

When we instantiate a KSPManager, its constructor calls LoadInstancesFromRegistry, which sets AutoStartInstance:

CKAN/Core/KSPManager.cs

Lines 422 to 430 in 00211bd

try
{
AutoStartInstance = Win32Registry.AutoStartInstance;
}
catch (InvalidKSPInstanceKraken e)
{
log.WarnFormat("Auto-start instance was invalid: {0}", e.Message);
AutoStartInstance = null;
}

However, the value passed is what AutoStartInstance would already return!

CKAN/Core/KSPManager.cs

Lines 25 to 36 in ad32e9e

public string AutoStartInstance
{
get { return Win32Registry.AutoStartInstance; }
private set
{
if (!String.IsNullOrEmpty(value) && !HasInstance(value))
{
throw new InvalidKSPInstanceKraken(value);
}
Win32Registry.AutoStartInstance = value;
}
}

So that assignment doesn't accomplish anything. It does throw an exception if the current auto start instance isn't valid, however, which is just caught and printed as a warning.

Changes

  • LoadInstancesFromRegistry no longer pointlessly assigns to AutoStartInstance
  • To guarantee identical behavior, AutoStartInstance now returns null if the value in the registry isn't the name of an instance
  • Win32Registry.SetRegistryToInstances no longer accepts the auto start instance as a parameter since all calls to it were just passing the value already in the registry
  • Some log messages that were redundant with exceptions are now removed to clean up test output
  • Spelling of "invalid" is fixed in a few places

Fixes #2774.

@HebaruSan HebaruSan added Bug Something is not working as intended Easy This is easy to fix Core (ckan.dll) Issues affecting the core part of CKAN Pull request Tests Issues affecting the internal tests labels Jun 10, 2019
@HebaruSan HebaruSan merged commit 4fcd298 into KSP-CKAN:master Jun 10, 2019
HebaruSan added a commit that referenced this pull request Jun 10, 2019
@HebaruSan HebaruSan deleted the fix/autostart-warning branch June 10, 2019 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Easy This is easy to fix Tests Issues affecting the internal tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'ckan compare' cmd warns of invalid autostart instance
2 participants