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

Project structure and folder cleanup #8

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

Conversation

MikeFarrington
Copy link

No description provided.

take in changes from original author
@MikeFarrington MikeFarrington marked this pull request as ready for review January 15, 2021 00:07
@MikeFarrington
Copy link
Author

@Ken98045 It looks big, with 138 files changed, but only a dozen files were actually changed, the rest were moved into folders and a few were slightly renamed.

This brings the project in line with a more traditional file/folder layout. It's not 100% there, but it's a good start.

@MikeFarrington
Copy link
Author

When you merge this, you'll probably want to do a "squash and merge" to keep the commit history a little cleaner. I should have done that myself... I'm still learning how to work on a forked project in GitHub.

@Ken98045
Copy link
Owner

Ken98045 commented Jan 15, 2021 via email

@MikeFarrington
Copy link
Author

Git/GitHub definitely has a steep learning curve, that's for sure. I only started using it about 4-5 years ago myself. Before that, it was all systems based on a check-in/check-out model.

@Ken98045
Copy link
Owner

Ken98045 commented Jan 15, 2021 via email

@MikeFarrington
Copy link
Author

I'm running it on Windows Server myself without issue.

I did run into one issue, and maybe it's the same as theirs...

After cloning your repo and grabbing a database to use, I found myself unable to run because the non-configurable connection string specifies a common default instance name of MSSQLLocalDB. I also had SQL LocalDB 2016 installed onto my development machine with that instance name. Thus, SQL 2019 LocalDB could not use it, and SQL 2016 LocalDB (which is what the program was connecting to) wouldn't allow the newer database.

Luckily I knew I had nothing of value that was using SQL 2016 LocalDB, and I went ahead and uninstalled it then changed the SQL 2019 LocalDB instance name to MSSQLLocalDB. Not everyone will have that luxury as they may be using software that needs that instance name.

That is also one of the reasons I wanted to move the project over to WiX Toolset so that the InstanceName could be specified during installation of SQL 2019 LocalDB. I imagine we'd want to use an instance name of OnGuard.

For this reason, the connection string needs to be moved to the registry to allow for modification. That way if someone else runs into this issue, they can create an different instance for 2019, then point the connection string to it.

Lastly, I know SQL Server throws up serious warnings when you attempt to install onto a Windows Server that is acting as a Domain Controller. I don't know if that's the case with the LocalDB edition as well. It could be that the installer failed to install SQL 2019 LocalDB for this reason. In which case, the end-user would need to download and run that installer manually, and tell the installer that they wish to install despite its objections.

@MikeFarrington
Copy link
Author

You may not be aware of how git works, but... when it comes to merging in my code, as long as it says there's no conflicts, then there's nothing manual about merging. You don't have to make any decisions or anything, just check the code for quality and bring it in. The "Bird" one is super-simple in that respect.

My bigger PR, this one here, I have "rebased" my changes onto yours. That means I have already sucked in your changes, handled the merge conflicts. What the rebase does is makes it look like ALL of my code was written after your code. It makes for much cleaner repo histories than willy-nilly merging.

So, if you look through the code in this PR, you'll see all of your changes for the registry settings. Don't look at the "changed files", that just shows my changes. Because of the aforementioned rebase, all you see is my changes. However, if you dive down into the branch itself (MikeFarrington:feature/FolderCleanup), not the PR, you'll see that your registry changes are there.

@MikeFarrington
Copy link
Author

| but then came Covid and I got really bored.

Yup, that's the only reason I've joined your project. I've never worked on an open-source project before because I've always been too busy with other stuff. Well, after gearing up with a bunch of cameras and building an NVR, I needed something "fun" to make it better. I saw your project, thought it had potential, and dove in.

@MikeFarrington
Copy link
Author

No need to squash, I finally learned how to squash locally in git (I've only ever relied on the squash capabilities in GitHub in repositories I control). So now everything is in one clean commit.

@Ken98045
Copy link
Owner

Ken98045 commented Jan 16, 2021 via email

@Ken98045
Copy link
Owner

Ken98045 commented Jan 16, 2021 via email

@Ken98045
Copy link
Owner

Ken98045 commented Jan 16, 2021 via email

@Ken98045
Copy link
Owner

Ken98045 commented Jan 16, 2021 via email

@Ken98045
Copy link
Owner

Ken98045 commented Jan 16, 2021 via email

@Ken98045
Copy link
Owner

Ken98045 commented Jan 17, 2021 via email

@Ken98045
Copy link
Owner

Ken98045 commented Jan 17, 2021 via email

@Ken98045
Copy link
Owner

Ken98045 commented Jan 17, 2021 via email

@Ken98045
Copy link
Owner

Ken98045 commented Jan 17, 2021 via email

@MikeFarrington
Copy link
Author

Sorry, I wasn't really online this weekend.

The build errors due to the missing DBMotionFiles is because your .gitignore file has exclusions for SQL .MDF/.LDF files on line 250. It is "bad form" to have large binary files in git. In order for me to start working on the project, I had to go grab these database files from my installation. This was one of the reasons I was talking about looking towards EntityFramework... it'll create the database on project launch.

I am using Visual Studio 16.8.4 (Professional Edition). Here's a Visual Studio tip for you... if things go really wonky like they did for you, you want to clear the visual studio cache for your solution. You need to delete the .VS subfolder in the solution folder (while Visual Studio is closed of course).

You mentioned that you did not see the Bird changes. That is because those are in a different pull request. It is good form to have isolated feature branches / pull requests.

@Ken98045
Copy link
Owner

Ken98045 commented Jan 18, 2021 via email

@Ken98045
Copy link
Owner

Ken98045 commented Jan 19, 2021 via email

@Ken98045
Copy link
Owner

Ken98045 commented Feb 3, 2021 via email

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.

2 participants