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

Treat reinstalling a changed module as an update #3828

Merged
merged 1 commit into from
Apr 19, 2023

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Apr 19, 2023

Background

If you install a module, and then we later change it in the metadata repo, you'll get this popup when you refresh your registry:

popup

If you click Yes, the module is reinstalled. If you click No, then it's not. The idea is to keep users up to date with the latest fixes and improvements as we maintain the metadata, such as missing dependencies or broken install stanzas.

Problems

  • This popup is confusing in general. What is it saying? Should I say Yes or No?
  • If you click No, there's no way to go back if you change your mind, other than refreshing again, which will only work if other modules in the repo have been changed in the meantime
  • Currently reinstalling does not reliably update the installed module's metadata in the registry (which is the whole reason we're doing it), so the popup keeps reappearing for the same mods.

Causes

I think this line from #3344 re-installs from the current/old metadata, not the new:

.Select(m => new ModChange(m, GUIModChangeType.Update))

ModuleInstaller.Upgrade makes promises about reinstalling, into which I read too much.

Motivations

@JonnyOThan had some questions about refactoring how CKAN installs RPM. While investigating, I had a thought:

image
(Don't worry, it wasn't actually that hard.)

The logic here is that we sometimes need to make related changes to the metadata, for example adding a new module and updating other modules to depend on it. It makes sense to apply such changes together in the same filesystem transaction, but currently we can't do that; reinstalling changed modules happens first, during repo update, and updating to new versions of modules happens separately. This allows users to end up with inconsistent metadata, some from before we merged a PR and some from after.

Changes

  • Now the repo update code no longer deals with module changes at all. The kraken class and the associated code, popup, and i18n resources for triggering a reinstall are deleted.
  • Now IRegistryQuerier.HasUpdate returns true if the metadata for an installed module differs from the same mod version from the repo, using the same comparison logic the repo update code used previously (which has been migrated to CkanModule.MetadataEquals)
    • This causes the upgrade checkbox to show up for such mods
      • If checked, the changeset will include an upgrade entry containing the module from the repo (if available) instead of the installed module, to ensure it is updated properly
      • If checked, the changeset will say "Re-install (metadata changed)" (with translations cobbled together from other strings where possible)

You can now freely refresh your registry and not worry about what "metadata changed" means. Any modules with changed metadata will be included in your next pass of updates, which will happen all together in one filesystem transaction. This will be easier for users (fewer steps and screens to learn) and better for maintainers (related changes will be applied together).

Fixes #3825 (I think, needs confirmation). @Silvia-Dragoness, if you have time to take a look at it, there should be a test build here under the Checks tab, Artifacts dropdown a few minutes after this is submitted.

@HebaruSan HebaruSan added Bug Something is not working as intended Enhancement New features or functionality GUI Issues affecting the interactive GUI Cmdline Issues affecting the command line Core (ckan.dll) Issues affecting the core part of CKAN Registry Issues affecting the registry labels Apr 19, 2023
@HebaruSan HebaruSan requested a review from techman83 April 19, 2023 00:06
Copy link
Member

@techman83 techman83 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Some tests on the comparison logic would be good, but I am not across the value vs implementation time here.

@HebaruSan HebaruSan merged commit 517059f into KSP-CKAN:master Apr 19, 2023
@HebaruSan HebaruSan deleted the fix/reinstall-changed branch April 19, 2023 01:50
@HebaruSan HebaruSan added the i18n Issues regarding the internationalization of CKAN and PRs that need translating label Apr 19, 2023
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 Cmdline Issues affecting the command line Core (ckan.dll) Issues affecting the core part of CKAN Enhancement New features or functionality GUI Issues affecting the interactive GUI i18n Issues regarding the internationalization of CKAN and PRs that need translating Registry Issues affecting the registry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Repeated "metadata has changed" messages
2 participants