Skip to content

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

Merged
merged 1 commit into from
Oct 17, 2019
Merged

Order directories by path #393

merged 1 commit into from
Oct 17, 2019

Conversation

JscNZ
Copy link
Contributor

@JscNZ JscNZ commented Oct 15, 2019

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.

image
Running Directory.GetDirectories(upPath) on windows vs linux

@erikbra
Copy link
Member

erikbra commented Oct 16, 2019

@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?

@JscNZ
Copy link
Contributor Author

JscNZ commented Oct 16, 2019

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:
image

@erikbra
Copy link
Member

erikbra commented Oct 17, 2019

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():

- When running on a non windows target (or non-ntfs volume) the directories returned are not always ordered correctly.
@JscNZ
Copy link
Contributor Author

JscNZ commented Oct 17, 2019

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.

@erikbra
Copy link
Member

erikbra commented Oct 17, 2019

Force push ftw! (as long as it's not on master ;)) Thanks a lot for your contribution!

@erikbra erikbra merged commit 7a82348 into chucknorris:master Oct 17, 2019
@erikbra erikbra added this to the 1.2.1 milestone Oct 17, 2019
@JscNZ JscNZ deleted the feature/OrderDirectoriesByPath branch October 17, 2019 22:00
@PawelBaranowski
Copy link

@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.

@erikbra
Copy link
Member

erikbra commented Apr 15, 2020

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 :/

@PawelBaranowski
Copy link

PawelBaranowski commented Apr 15, 2020

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.

@erikbra
Copy link
Member

erikbra commented Apr 15, 2020

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

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