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

Feature/change file structure #110

Merged
merged 5 commits into from
Aug 18, 2024

Conversation

TOomaAh
Copy link
Contributor

@TOomaAh TOomaAh commented Aug 10, 2024

No description provided.

@henrygd
Copy link
Owner

henrygd commented Aug 10, 2024

Thanks!

I only had a quick minute to look over it, but seems like a nice improvement! Should make it easier to add tests as well.

Just a couple things:

I'd like to avoid having lots of files pile up in the root directory, so perhaps we can make a folder called beszel or app in the root directory and move the application related code there.

Also, I noticed you accidentally committed an exe file. Sorry to ask you to do this, but I'd like to remove that from the git history. Not sure of the best way to do this. Maybe either git reset --soft and git push --force, or just making a fresh pull request.

Anyway, thanks again. I'll fiddle around with the workflows to make sure those will still work, then merge when I have a bit more time over the next few days.

@TOomaAh TOomaAh force-pushed the feature/change-file-structure branch 3 times, most recently from e075310 to b51770a Compare August 11, 2024 00:17
@TOomaAh
Copy link
Contributor Author

TOomaAh commented Aug 11, 2024

Thanks!

I only had a quick minute to look over it, but seems like a nice improvement! Should make it easier to add tests as well.

Just a couple things:

I'd like to avoid having lots of files pile up in the root directory, so perhaps we can make a folder called beszel or app in the root directory and move the application related code there.

Also, I noticed you accidentally committed an exe file. Sorry to ask you to do this, but I'd like to remove that from the git history. Not sure of the best way to do this. Maybe either git reset --soft and git push --force, or just making a fresh pull request.

Anyway, thanks again. I'll fiddle around with the workflows to make sure those will still work, then merge when I have a bit more time over the next few days.

I removed the executable from the git history.

I didn't understand the second point. Do you want me to remove the 'internal', 'cmd', etc... folders from the project root or do you want me to remove the 'Dockerfile...' files or both?

@henrygd
Copy link
Owner

henrygd commented Aug 11, 2024

Sorry, I mean both.

This is more personal preference than common practice, but I'd like for the root directory to have fewer files and clear separation of purpose, rather than having everything intermingled at the base.

So I'd prefer to have the same files at the root as the repo currently has, but instead of individual folders for hub / agent, we just have one folder named beszel, and the code for the app lives there.

We can keep the structure that you have currently, it just goes in the beszel folder instead of the root.

If this still isn't clear -- and I understand why it may not be -- I can make a commit tomorrow that organizes that way.

@henrygd
Copy link
Owner

henrygd commented Aug 11, 2024

I made some updates and submitted a pr on your fork.

If you think of any other changes, you can merge and update. Otherwise I can just continue working on the branch I checked out already.

@TOomaAh
Copy link
Contributor Author

TOomaAh commented Aug 11, 2024

For my part, I won't go into any further modifications or code slicing to avoid creating bugs where none exist.
Let me know when you want me to merge your modifications into the PR and I'll do it.

@henrygd
Copy link
Owner

henrygd commented Aug 12, 2024

No worries, no need to merge if you're done. I'll close that pr and eventually merge this to main after I've had a bit more time to look it over and test workflows.

@henrygd henrygd merged commit ea71492 into henrygd:main Aug 18, 2024
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