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

Feature mode copy and code refactoring #1117

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

urban-1
Copy link

@urban-1 urban-1 commented Jan 17, 2018

Hi @lra / all,

This is an attempt to introduce copy mode in Mackup. However, while going through the code I could not resist restructuring things a bit... So I will explain this PR in two main sub-sections:

Code Refactoring

The changes are mainly focused on defining clear interfaces between components and thus code has been moved around but not changed a lot. The most notable changes are:

  • Most of Mackup functionality should be provided by the Mackup class which serves as an entry point to mackup api and acts as a controller for individual commands. Mackup now holds:
    • The global configuration (as before)
    • The runtime parameters
    • The application database
    • The user's environment
  • The ApplicationsDatabase is now loading ApplicationProfile instances instead of dicts. Helper methods have been removed and replaced by __getitem__ implementation which allows [<appname>]. This way the Mackup instance (and previously the main.py) does not need to loop over the DB and create profiles, it just loops and applies the operation requested.
  • The ApplicationProfile is mostly the same. One difference is that instead of passing individual config or runtime values (like dry_run) down to its methods, we now use the Mackup instance (which contains all of those). Although a minor modification, allows for more attributes to be added into Mackup (and thus the cmd line args or global config) without modifying the ApplicationProfile method signatures...

I wanna believe that most of the refactoring makes senses and makes the code easier to extend and maintain.

Copy Mode

The implementation of the copy mode is a global one as opposed to a per-INI/app. This simplifies the code, keeps config-file changes to minimum and avoids few issues that need resolving. These include:

  • How do we deal with apps that can be both linked and copied? This is a user preference which as you said should be stored in the INI... but doing so makes that INI file not generic any more (since it contains a user preference).
  • One could introduce a section in the global config that dictates which apps should be backed up via copy instead of linking. This will complicate the logic in the ApplicationProfile, especially given that a user might change configuration of an already backed up app! The implementation of this tho with the new code is pretty straight forward (ApplicationProfile.backup can override mode)
  • I cannot come up with a valid case where one needs some apps linked and some apps copied but (over)thinking about it, this can be extended down to a per-file manner!!!

Due to the above I believe that a global configuration option is a fast win and the safest way to go. We will need to think of the reconfiguration problem and how we detect it (Mackup metadata file in storage, including version, mode, etc?)

Merging

I understand that such code changes are risky, especially given the nature of this app. If you review the code and decide that we should proceed with a merge, I would suggest that a new branch is created on the upstream repo (lets say rc-0.9.0):

  • We merge my feature_copy_mode to rc-0.9.0
  • New application profiles will always be added to the old upstream 0.8.x
  • 0.8.x will only receive bug fixes (if any)
  • All new features go to rc-0.9.0
  • Devs and testers work with rc-0.9.0
  • Once rc-0.9.0 is deemed stable, we merge it to master

Some conflicts are to be expected on any code changes in 0.8.x but these shouldn't be too many.

Notes:

  • Still WIP but some experimentation with mackup show git feature is included. This breaks atm the clean API and needs to be defined better and moved into Mackup class...

Let me know what you think and if we should proceed with merging.

Cheers,

Andreas

@MadeOfMagicAndWires
Copy link
Contributor

MadeOfMagicAndWires commented Jan 17, 2018

To be clear, I haven't looked at the code yet, as it's a bit much for right now, but some points missing from the overview:

So, just for clarity, what does copy mode do 😏? Is it meant to replace symlinking in backup as suggested in #859 or is it just used as a one time command to copy files?

Second, how does it handle filename conflicts? Say there's an older copy in the backup folder already
and I'm now running backup again. Say I've symlinked the files already and now decide I'd rather want copies?

@urban-1
Copy link
Author

urban-1 commented Jan 17, 2018

The copy mode replaces the current linking mode. This is enabled by adding the following in ~/.mackup.cfg:

[general]
mode = copy

The commands remain the same, with exception the mackup uninstall which is a dummy one and has no effect in this mode (just prints a message):

  • mackup backup will load all application profiles and copy every file listed to the backup location.
  • Similarly mackup restore will copy from the backup location into the home directory overwriting any existing files. restore will warn if a file already exist and will prompt the user (displaying relevant date information).

Copy operations are implemented with distutils.dir_util.copy_tree(src, dst, update=1):

If update is true, src will only be copied if dst does not exist, or if dst does exist but is older than src.

At the moment there is no detection on if the user changed configuration from link to copy. This is something that I can easily add (when I find some time) in order to avoid disasters... However, I would like to see if you are willing to consider merging before adding final touches :) (I am using it in copy mode on my boxes atm)

My time is limited but I will do my best to reply on this thread. I tend to have more time in weekends for coding/fixing. Let me know if you require more information.

@MadeOfMagicAndWires
Copy link
Contributor

Thanks for the quick reply. Seeing how intensive this PR is I think I'll leave the decision to @lra but good work nonetheless

@xkef
Copy link

xkef commented Dec 27, 2018

As mentioned in #859 how is this better than directly using rsync?

@urban-1
Copy link
Author

urban-1 commented Dec 27, 2018

Hey @xkef, although I don't see this PR ever merging, your sh implementation would also do as minimal implementation. However, you would need to extend with the restore (and maybe show) functionality. Two things to keep in mind:

  1. There was talk in the past regarding detecting and encrypting passwords when we back up (not sure if this was implemented). In this case the shell script would need to do the same which I suspect would require more complex config parsing

  2. Altho I started back then implementing a copy feature, at the end I refactored lots of code... I believe this is why this will not be merged (maybe high-risk?). So other, than introducing a copy feature, this was also an attempt to make Mackup internal API cleaner

@xkef
Copy link

xkef commented Dec 28, 2018

ty @urban-1 for your insights.

I guess the only value (except for the nicely written python code) in this repo is the applications db. However, it still is not handling my standard configs of vscode insider versions or non-std dotfiles paths well - it is very opinionated in this respect. I don't even want to mention handling complex zsh/vim configs with deep symbolic / hardlinks...

I will stick with my ansible orchestration combined with rsync and git for synchronization.

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.

3 participants