search

sepharnaert / hogent-bachelorproef-src

0 stars 0 forks source link

Vagrantfile (Code Review) #2

Open AntonVanAssche opened 5 days ago

AntonVanAssche commented 5 days ago

I'm not entirely sure what you're aiming to achieve here, or what is the reasoning behind some of the steps you are doing, so I’d like to clarify a few points:

  1. Regarding software-properties-common: The software-properties-common package appears to be specific to Debian/Ubuntu-based systems. Since this virtual machine is a Fedora system, this package is not applicable here. I'm running Fedora and can't find it within the base repositories, or am I missing something here?
$ sudo dnf search software-properties-common
Last metadata expiration check: 0:18:41 ago on Tue 26 Nov 2024 11:47:56 PM CET.
No matches found.

  1. Regarding dnf-add-repository: On line 28, the use of dnf-add-repository seems incorrect, as this command doesn't exist AFAIK. The equivalent would be dnf config-manager, which requires the dnf-plugins-core package to be installed first, but isn't needed here since Ansible is present in the standard repos. That said, would it make more sense to install Ansible using pip? This approach tends to provide a version more aligned with the latest upstream releases.

  2. Regarding sudo: If I’m not mistaken, the shell provisioning script is executed as the root user, so the sudo commands appear redundant. Removing them might improve clarity and simplify the script. Of course, let me know if I’m mistaken here.

  3. Regarding nmcli: The network settings specified using nmcli appear to duplicate what Vagrant is already managing through the config.vm.network directive. It might be better to rely on Vagrant’s built-in networking to avoid potential conflicts?

  4. Regarding OS Choice: Since the goal is to simulate a server environment, you might consider using a more commonly used server OS, such as AlmaLinux, if you want to stay within the RHEL-based ecosystem. Fedora is a great choice for cutting-edge development but is less typical in production environments AFAIK.

  5. Extending of disk: The use of growpart and resizing commands suggests that the disk is not fully utilized by default. This seems unusual to me, as Vagrant typically handles disk allocation and partitioning automatically. Was this disk added manually? If so, it’s not reflected in the current Vagrantfile configuration, which might make it harder for others to understand or reproduce the setup. If the disk resizing isn’t strictly necessary, this step could potentially be omitted to simplify the script.

https://github.com/sepharnaert/hogent-bachelorproef-src/blob/8c3cdba7fbe2af50db2928ab487174a604d89808/Vagrantfile#L23-L37

sepharnaert commented 3 days ago

1 and 2. I'll look in to it, this may have been copied form a forum or AI tool that added it and I over looked it. If it still works without I' ll remove the unnecessary lines to clean up the code.

  1. Regarding sudo, you are right, the shell provision is executed as root. I removed the sudo commands, the reasson they were in there is because of me manually testing the commands using sudo and copy pasting them into the file.

  2. NMCLI is needed, the vagrant config command seems to just not work every time I created the VM it gave another IP. I tried lots of different possibilities, but non seem to work. The original documentation command is the one I use, but for some reason it doesn't seem to do as I want. Therefore I added the commands at the end.

  3. The OS choice is made so to level the playfield between the VM and the container. If both run the same OS, there can't be any difference on that part hence why to choice of using Fedora. I know its not the most optimized OS for servers.

  4. The disk is not added manually, but configured with the Vagrantfile according to their documentation it should work. When i opened the VM it had the allocated space given to the VM but it didn't allow it to be used, when installing GitLab using the ansible file it gave the error that it ran out of usable space. The only way i found to omit this was by adding the commands to enlarge the disk.