ENT-14408, CFE-4700: Added option to retain git history when using cfbs convert & Replaced cp/rsync with shutil.copy - #332
Conversation
…nputs Ticket: ENT-14408 Changelog: Title Signed-off-by: Simon Halvorsen <simon.halvorsen@northern.tech>
8f917ce to
fcd6dfe
Compare
olehermanse
left a comment
olehermanse
left a comment
There was a problem hiding this comment.
Looks like you're moving the existing repo into a .old directory? And then adding that to gitignore? That's not what we mean by retain, we want the new project (after conversion) to have (retain) the git history of what happened before conversion.
Essentially, we want cfbs convert to do something equivalent to what I've done in a shell here;
$ cd my-masterfiles
$ ls -a
.git/ promises.cf update.cf [...]
$ mkdir ./my-masterfiles # We could prompt about what the name of the subdirectory should be
$ mv promises.cf update.cf [...] ./my-masterfiles
$ git add ./my-masterfiles
$ git commit -m "Moved CFEngine policy related files to subdirectory to convert this repository into a CFEngine Build project"
$ cfbs convert # (init + add +++)
The existing code (in cfbs convert / init) might not work correctly when .git already exists, in that case we need to fix that.
Yeah, that makes much more sense now that I think about it. Originally I thought it more as a 'backup' inside the the build-project, but I see what you mean. |
fcd6dfe to
488603b
Compare
Ticket: CFE-4700 Changelog: Title Signed-off-by: Simon Halvorsen <simon.halvorsen@northern.tech>
488603b to
a8b63ac
Compare
larsewi
changed the title
| pass | ||
|
|
||
|
|
||
| def copytree_merge(src, dst, ignore=None): |
There was a problem hiding this comment.
You can mention in the commit message that this was copied from cf-remote
| @@ -1208,9 +1211,27 @@ def analyze_command( | |||
|
|
|||
| @cfbs_command("convert") | |||
| def convert_command(non_interactive=False, offline=False): | |||
There was a problem hiding this comment.
We should really think about refactoring this function (400 lines of spagetti).
| # a pre-existing `.git`-directory was moved or removed below, | ||
| # so restore the original directory (including its `.git`) from | ||
| # the backup we took before touching anything | ||
| print("Restoring '%s' to its original state..." % path_string) |
There was a problem hiding this comment.
Would be helpful for the sake readability to pass this string as an argument to the function.
| # back up the directory before doing anything destructive to it, so | ||
| # a failure can restore it to its original state instead of leaving it half-converted |
There was a problem hiding this comment.
Maybe you can move this logic to a separate function so that this function does not get even longer. And avoid defining it inside this function like the others. It's problematic, because they have access to variables in the outer scope. Which can have unintended side effects that are really hard to debug.
| cp(backup_dir, path_string) | ||
| rm(backup_dir, missing_ok=True) |
There was a problem hiding this comment.
Should probably sync the directory after cp and before rm. Otherwise the files disappear in e.g. a power outage
| non_interactive, | ||
| ) | ||
| except: | ||
| cfbs_convert_cleanup() |
There was a problem hiding this comment.
Maybe we should check these things up front before doing anything?
3 participants
No description provided.