Closed JscNZ closed 4 years ago
@JscNZ Thanks a lot for you contribution. Just to be clear, 0010_Add_orderedParts
, 0015-PRIC-154
, etc, are directories in the up
folder, not files?
I think maybe the configuration flag is not necessary. Even though not explicitly stated that directories should be read in alphabetical order, I think the documentation here implies that directories should also be read alphabetically.
https://github.com/chucknorris/roundhouse/wiki/GettingStarted#up---dmlddl-updates
So, I argue that the directories not being sorted alphabetically on Linux is simply a bug, and we could skip the configuration altogether. Do you agree?
Thanks @erikbra yeah to confirm those are directories (each containing upscripts). If you look at DotNetfileSystemAccess class the methods like get_all_file_name_strings_in(string directory, string pattern)
to get the filenames in a directory already order the script names - it's only retrieving the list of directories that seem to be affected.
I'm totally fine without a configuration flag and handling it as a bug. My only concern was that with this change if you in theory had used roundhouse to build a DB on prior version, then run that same set of scripts to build another DB you'd potentially get a different run order if you're running on linux.
Here's the full screenshot of the old behaviour including the order it was running the scripts:
Thanks for the clarification. I think it's not worth the effort/complexity to have a configuration flag on this, I see you point, but the "order" that the scripts are in in master now on Linux seems just random, and I don't think there is a big chance that anyone relied on this order.
Could you please remove the configuration switch, and just always do
Directory.GetDirectories(directory).OrderBy(d => d).ToArray():
Awesome it's done. You're totally right - thinking about it a little more having a configuration flag that's only used to correct a bug seems a bit backwards!
I just force pushed the branch 🙈 hopefully not an issue for you guys.
Force push ftw! (as long as it's not on master ;)) Thanks a lot for your contribution!
@erikbra Can we please release 1.2.1? #393 is essential for proper subdirectories handling on non-NTFS filesystems. #386 makes it impossible to switch to Linux for at least two people (@domingoladron and me) and I'm sure there are more.
Yes, sure, I can try to get it out. Sorry for the lack of response in general lately, there has been a lot of other stuff on my agenda too :/
You never know when life hits you. Tell me if I can be of any help. BTW is there any discussion about current state of RH and future plans? Is there anything on the roadmap or is it in maintenance mode? I have some ideas for PRs but am not sure if it's worth to invest my time.
Thanks for your concern!
Good question on RH, really. I really want to move it forward, but I struggle a bit with no one else (almost) contributing. There are a few PRs here and there, which I appreciate a lot, but especially on some of the DB providers we are really lacking competence (especially Oracle). I want to move forward to .NET Core 3.1 (and soon-ish .NET 5), as the options for deployment are much better there (modern-style packaging as single file, e.g), but there are some technical dependencies we need to get rid of before going there (Log4Net, NHibernate). I should probably try to make some issues for each of these, to make it easier to separate the work
As seen in #386 directories are not ordered when they're retrieved. This results in different behaviour when executing up scripts on windows vs linux (assuming it's actually different due to the way the different filesystems work under the hood).
This adds an option to force the directories to be retrieved in order so that scripts run in a repeatable manner. The config defaults to false to avoid breaking any existing executions based on the old behaviour.
Directory.GetDirectories(upPath)
on windows vs linux