-
Notifications
You must be signed in to change notification settings - Fork 246
Order directories by path #393
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
Order directories by path #393
Conversation
@JscNZ Thanks a lot for you contribution. Just to be clear, 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 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
|
- When running on a non windows target (or non-ntfs volume) the directories returned are not always ordered correctly.
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. |
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.
Running
Directory.GetDirectories(upPath)
on windows vs linux