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

Add support for Ubuntu 16.04 LTS #214

Merged
merged 1 commit into from
Apr 10, 2018
Merged

Conversation

jkroepke
Copy link
Contributor

@jkroepke jkroepke commented Apr 10, 2018

Pull Request (PR) description

I'm using this module long time on Ubuntu 16.04 without any errors.

This Pull Request (PR) fixes the following issues

I'm using this module long time on Ubuntu 16.04 without any errors.
@juniorsysadmin juniorsysadmin merged commit 514d2e8 into voxpupuli:master Apr 10, 2018
@LongLiveCHIEF
Copy link
Contributor

@jkroepke does the module resolve to setting up a systemd service or init.d on your 16.04 installation?

@LongLiveCHIEF
Copy link
Contributor

I'm going to leave this in master for the next release, but the first issue we get about ubuntu not working with a gitlab upgrade or service restart then I'm going to revert and re-deploy. I don't think there will be any, but I didn't get the time to really look/think before a voxpupuli org member merged it.

@juniorsysadmin in the future please give the module's team time to properly review a PR and think through it/vote on it before merging it. I can understand if this merge was several days old and had no activity from a team member, but you just merged a "works on my machine" PR that adds no new tests, less than 24 hours after it was submitted.

I think there may be one or two additional unit tests needed to ensure continuous delivery for Ubuntu 16.04 over Ubuntu 14, due to the nature of the override service start script for the two different init providers for these operating systems.

I would have preferred to verify this before it was merged.

@LongLiveCHIEF
Copy link
Contributor

@jkroepke btw, I do appreciate the PR, and I know you've submitted several now, so thanks for helping us maintain and keep this up to date. I just want to make sure we aren't moving forward recklessly.

@jkroepke
Copy link
Contributor Author

jkroepke commented Apr 11, 2018

@LongLiveCHIEF After installing the gitlab-ce deb package, running the gitlab-ctl reconfigure defines/provde a systemd unit file. So this module must not provide a own unit file.

Maybe we miss a dependency Exec['gitlab_reconfigure'] -> Service['gitlab-runsvdir] but that not a specific Ubuntu 16.04 issue.

@jkroepke
Copy link
Contributor Author

About

but you just merged a "works on my machine" PR that adds no new tests

Is this statement correct? I thought on_supported_os.each do |os, facts| loop over all defined or supported operating system. By adding Ubuntu 16.04 to the support matrix, the spec test should run test with ubuntu 16.04 facts, too. Right?

That was a (one of two) reason why I add no new tests.

@jkroepke jkroepke deleted the patch-1 branch April 11, 2018 20:24
@LongLiveCHIEF
Copy link
Contributor

LongLiveCHIEF commented Apr 11, 2018

Is this statement correct?

In most modules yes. This one is a little wonky though, because it's a puppetization of a chef run via custom service commands for a unified service of a conglomerate of contained services.

WHEW. I need a drink.

Even with all the above, we're most likely covered by the iteration of supported OS's in the existing unit tests, but the unique qualities of this module in combination with a different service provider for the Ubuntu OS from one major version to the next has me exhibiting an abundance of caution.

There are some specific upstart stuff in this module that are specific to ubuntu, but were added to the module during the days when it was assumed Ubuntu did not use systemd, and RHEL family systems did.

I want to rule out some likely scenarios with survivability of host reboots and package upgrades before officially adding support for Ubuntu 16+ using the existing codebase.

@LongLiveCHIEF
Copy link
Contributor

*wanted to rule out

@jkroepke
Copy link
Contributor Author

WHEW. I need a drink.

Grab some 🍺 :)

Thanks for the clarification.

There are some specific upstart stuff in this module that are specific to ubuntu

Now I know what did you about the service problematic. The module override alls start/stop/restart commands, so it ignore the service provider completly? Ughs.

Also it looks like the special init.d/upstart handling you also maybe talking about is gone.

@LongLiveCHIEF
Copy link
Contributor

It has to override the start/stop/restarts, because with gitlab-omnibus, the gitlab-runsvdir service is not a master service. Without any overrides, it will only bring up nginx/rails, postgres, and redis. That leaves at least half the other "embedded services" for this package offline.

@LongLiveCHIEF
Copy link
Contributor

Also it looks like the special init.d/upstart handling you also maybe talking about is gone.

🤦‍♂️ I forgot about that, and it was a commit I authored, and I did it specifically to prepare for supporting newer Debian releases.

I think we should be good to go (now that you reminded me I already solved the problem I'm currently worried about.)

yep, it's Oh-:beer:-thirty.

👍 💯 to Ubuntu 16.04 support

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.

3 participants