search

ej52 / proxmox-scripts

MIT License
587 stars 308 forks source link

Why are you removing dependencies on upgrade? #38

Closed Eeems closed 11 months ago

Eeems commented 3 years ago

Script nginx-proxy-manager

Describe the bug https://github.com/ej52/proxmox-scripts/blob/main/lxc/nginx-proxy-manager/install/alpine.sh#L64 You are removing dependencies on upgrade, just to install them again later. It would make more sense to just leave them installed. Is there a specific reason they need to be removed and then added again?

ej52 commented 3 years ago

@Eeems dependencies are removed to keep the proxmox container lightweight, they are only needed when installing NPM requirements.

Eeems commented 3 years ago

@Eeems dependencies are removed to keep the proxmox container lightweight

I think you failed to read my issue fully. They are removed, and then added again: https://github.com/ej52/proxmox-scripts/blob/main/lxc/nginx-proxy-manager/install/alpine.sh#L93

ej52 commented 3 years ago

Apologies, I did miss read the issue.

I added that bit of code as a way to remove any old dependencies that may have been installed with older installs of NPM using the script. As it would cause conflicts with NPM changes.

eg. NPM started using pip certbot package instead of apk package.

When the script finishes or exits unexpectedly, trapexit is called to cleanup and remove all the DEVDEPS to keep the proxmox container lightweight.

Eeems commented 3 years ago

pologies, I did miss read the issue.

I added that bit of code as a way to remove any old dependencies that may have been installed with older installs of NPM using the script. As it would cause conflicts with NPM changes.

eg. NPM started using pip certbot package instead of apk package.

You probably don't need to include $DEVDEPS in the remove list there since you'll be installing them again after upgrading all packages. Since $DEVDEPS doesn't include old dependencies that aren't needed anymore it would make sense to just include known conflicting packages like certbot in the list.

When the script finishes or exits unexpectedly, trapexit is called to cleanup and remove all the DEVDEPS to keep the proxmox container lightweight.

Fair enough. I would recommend adding a flag to disable this cleanup for those of use who don't care about them remaining installed for future updates.

ej52 commented 11 months ago

--cleanup flag added in the latest PR, by default it will leave all the dev dependencies, unless the flag is set.