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

made QoL adjustments and file cleanup #159

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

Conversation

brimdor
Copy link

@brimdor brimdor commented May 10, 2024

  • replaced specific unique items with placeholders (ex. khuedoan.com is now )
  • added a placeholder for control plane endpoint
  • added reset script
  • added cronjobs
  • added nfs_utils
  • adjusted the configure script to utilize placeholders
  • added a branch option in case someone wants to use a different branch than master
  • removed old configure and added new configure script

- replaced specific unique items with placeholders
- added reset script
- added cronjobs
- added nfs_utils
- adjusted the configure script to utilize placeholders
- added a branch option in case someone wants to use a different branch than master
renamed new file to "configure" and removed old configure file
- adjusted tools in Makefile to add the safe.directory argument to the docker run command in order to fix the error regarding ownership (Refer to khuedoan#139)
@brimdor brimdor mentioned this pull request May 10, 2024
@khuedoan
Copy link
Owner

khuedoan commented May 13, 2024

Hi, thank you for making a PR. However, it seems like this should be split into multiple PRs to make it more reviewable and reduce the blast radius.

Although these changes are unlikely to be merged for the following reasons:

  • Placeholders: Using khuedoan.com is intentional. This project is meant to be applied directly in my homelab, with a script to make it more convenient to adapt to your homelab. It's not meant to be a template repo.
  • Reset script: This is unnecessary since the nodes are designed to be cattle, not pets. To reset them, you can simply turn them off and run make to rebuild (same thing for the repo, you can nuke the entire repo and fork it again)
  • Cronjobs: Shouldn't this already be covered by the automatic upgrade?
  • nfs_utils: I'm assuming this is for RWX volumes, does the standard-rwx storage class suffice? (It was added as part of the Ceph migration)
  • Branch option: I don't plan to change my default branch, and it will complicate the installation instructions (e.g., "if you want to change the branch, you have to fork, checkout a new branch, change the default branch in GitHub, change this and this, etc."). It unnecessarily increases friction without any benefit.

@brimdor
Copy link
Author

brimdor commented May 13, 2024 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.

None yet

2 participants