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

Updating linux installer script #858

Merged
merged 1 commit into from
Jul 2, 2019
Merged

Updating linux installer script #858

merged 1 commit into from
Jul 2, 2019

Conversation

jalavoy
Copy link
Contributor

@jalavoy jalavoy commented Jun 15, 2019

This brings the installer script up to date and changes a few logical problems. For example running apt 50 times is a ton slower than running it once, same with dnf.

the yum install for RHEL was also installing 1777 undeeded perl and lua modules for no reason. Now it installs 53.

@mackal mackal requested a review from Akkadius June 15, 2019 18:21
@Akkadius
Copy link
Member

Hey @jalavoy thanks for taking the initiative

The reason why we didn't include the installs on one line is in the case specifically for Debian - the same lines are used for Ubuntu and Debian. because of this and distributions changing a package would go missing and then bomb the entire install, OR we have a package listed in the list that is available on Debian but not in Ubuntu and would break one and not the other. At least with the individual lines it may error but continue in the script. Yes, there is the apt-get --ignore-missing flag but we also had inconsistent behavior with that flag working as well so it wasn't a huge deal to list the commands out line by line, it makes it readable and maybe costs a few more seconds at most. For this reason I would want the Debian / Ubuntu block to the remain the same

Because Fedora and Redhat are definitely more targeted, I'm fine with it being in one command but I would prefer each package to be on its own line like:

dnf -y install \
	cmake \
	open-vm-tools \
	vim

I also would like to see proof that the final state of your PR runs through a 100% successful, operational server install. Each time we've made tweaks to this with new OS's we go through the whole routine

I would also like to make note that there are plans to revise some of the installer in the near future, most of it will remain the same but we have some changes to be incorporating soon

Kind regards,
Akk

@jalavoy
Copy link
Contributor Author

jalavoy commented Jun 15, 2019

This all sounds reasonable to me. I’ll update my end later tonight.

How would you like me to demonstrate the changes are working? Or is that done in your CI pipeline?

@Akkadius
Copy link
Member

Our testing has always been manual simply because testing the same containers over and over only does so much. We only usually run into issues when distribution upgrades happen, even then they may be in the middle of changing packages around and may or may not be final when that happens so its not something that can be simply automated anyways

We usually bring up VM's on our nodes and just run the scripts and check them in a while, make sure we can launch world / zone and everything is functional in game with scripts etc.

That being said I will probably at least look at hooking up a series of distributions to go through it all through a CI at some point so we at least have that as a measurable pass and run it through some basic automated tests. It's just never been a huge priority and has worked for thousands of installs without issues

fixing tabs

adding perl-json

adding sodium

adding openssl

removing debian stuff, refactoring rhel stuff

changing some fedora stuff

more fedora crap

dnf doesn't like newlines in group install?
@jalavoy
Copy link
Contributor Author

jalavoy commented Jun 16, 2019

I went ahead and spun up brand new Centos 7 and Fedora 30 VM's and applied the current version of the PR against them. Both of them built without errors and resulted in a running server.

Also reverted the apt changes for you.

This iteration should probably be good to go. Let me know if anything else needs tweaking.

@Akkadius
Copy link
Member

Hey @jalavoy - not ignoring this, I had surgery last week and then not long after recovering I drove 6 hours north out of cell range and now I'm back into the business week

It's on my list to run through the Ubuntu/Debian side of this and merge this in

Thank you!

@jalavoy
Copy link
Contributor Author

jalavoy commented Jun 25, 2019

No worries my dude. Lmk if you need me to update anything. Hope you’re feeling better.

@Akkadius Akkadius merged commit 6757f61 into EQEmu:master Jul 2, 2019
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