Cleanup package scripts

1. Remove the use of posttrans script for RPMs. We can handle them in postinst itself using value of $1
2. Run the postinst script only if it is a successful install/upgrade. For DEB based distros, that means $1 will be configure. For RPM based distros that means $1 will be 1 or 2 for install and upgrade respectively. This is needed because DEB-based and RPM-based distros handle failure of preinst differently. In RPM based distros, if %pre script fails, the installation/upgrade is aborted. However, in DEB-based distros, if preinst fails, the execution moves as preinst => postrm abort-upgrade => postinst abort-upgrade. That means, even if preinst fails, postinst is being called. So the stuff in postinst script gets executed even if preinst fails. Moving them to inside the conditional will fix it.

References:

1. https://fedoraproject.org/wiki/Packaging:Scriptlets
2. https://www.debian.org/doc/debian-policy/#maintainer-script-flowcharts

Tests:

• CentOS 6
  • Fresh Installation
  • Upgrade from already installed package
• CentOS 7
  • Fresh Installation
  • Upgrade from already installed package
• Debian 7
  • Fresh Installation
  • Upgrade from already installed package
• Debian 8
  • Fresh Installation
  • Upgrade from already installed package
• Debian 9
  • Fresh Installation
  • Upgrade from already installed package
• OpenSUSE 42.2
  • Fresh Installation
  • Upgrade from already installed package
• Ubuntu 14.04
  • Fresh Installation
  • Upgrade from already installed package
• Ubuntu 16.04
  • Fresh Installation
  • Upgrade from already installed package

changed the description

marked the checklist item Fresh Installation as completed

marked the checklist item Upgrade from already installed package as completed

marked the checklist item CentOS 6 as completed

@marin I am testing all the packages, so it is still WIP. But, can you take a look and see if this MR makes sense? Especially the second point of how failing preinst doesn't actually abort the process in Debian-based distros. (Read that in the context of PG 9.2 check)

marked the checklist item CentOS 7 as completed

marked the checklist item Fresh Installation as completed

added 1 commit

• e5667b39 - Explicitly exit preinst

Compare with previous version

added 1 commit

• 642a9d12 - Fix typo

Compare with previous version

marked the checklist item Upgrade from already installed package as completed

added 1 commit

• aa472d71 - Fix comment

Compare with previous version

resolved all discussions

In the description, I said this

DEB-based distros, if preinst fails, the execution moves as preinst => postrm abort-upgrade => postinst abort-upgrade. That means, even if preinst fails, postinst is being called. So the stuff in postinst script gets executed even if preinst fails. Moving them to inside the conditional will fix it.

My initial reasoning was, users will still see the messages in postinst/upgrade (the tanuki logo and gitlab ascii art) even if preinst failed and that wasn't good. And I thought that can be fixed if the postinst stuff was executed only if $1 was upgrade.

But, how Debian's maintainer script works is as follows. Consider upgrading foo from 1.2.3 to 1.2.4.

1. If preinst of 1.2.4 succeeds, it will in the end invoke postinst of 1.2.4
2. But, if preinst of 1.2.4 fails, it will in the end invoke postinst of 1.2.3, not 1.2.4.

That means, even if we put those stuff inside conditionals in the next version, 10.0, it will have that effect only if the upgrade is happening from 10.0 and preinst fails.

But still, it doesn't hurt to put them inside conditionals as it's just cleaner code.

mentioned in issue #2624 (closed)

added 1 commit

• e134cc9a - Revert "Explicitly exit preinst"

Compare with previous version

mentioned in merge request !1888 (merged)

added 1 commit

• e9141073 - Cleanup script

Compare with previous version

marked the checklist item Debian 7 as completed

marked the checklist item Debian 8 as completed

marked the checklist item Debian 9 as completed

marked the checklist item OpenSUSE 42.2 as completed

marked the checklist item Ubuntu 14.04 as completed

marked the checklist item Ubuntu 16.04 as completed

marked the checklist item Upgrade from already installed package as completed

marked the checklist item Fresh Installation as completed

marked the checklist item Upgrade from already installed package as completed

marked the checklist item Fresh Installation as completed

marked the checklist item Upgrade from already installed package as completed

marked the checklist item Fresh Installation as completed

marked the checklist item Fresh Installation as completed

marked the checklist item Upgrade from already installed package as completed

marked the checklist item Upgrade from already installed package as completed

marked the checklist item Fresh Installation as completed

marked the checklist item Upgrade from already installed package as completed

marked the checklist item Fresh Installation as completed

unmarked as a Work In Progress

assigned to @marin

@balasankarc There was a very valid reason why posttrans scriplet was used. I can't find it quickly and that requires research. I don't feel comfortable proceeding with this until we confirm that the original issue that this change was fixing is addressed. I very much doubt that anything changed here.

assigned to @balasankarc

@marin I believe it was introduced to handle the change of naming from "gitlab" to "gitlab-(ce|ee)" and the issue of symlinks being removed as part of postuninstall script. It was needed for upgrades from versions prior to 7.3. Ref: https://gitlab.com/gitlab-org/omnibus-gitlab/merge_requests/402

The question is, should we still support that? If we already have pit stops in the upgrade path (i.e if you need to upgrade to x.y.z, you should first need to upgrade to m.n.o), like with the PG9.2-removal change, it should already cover this.

/cc @twk3

mentioned in merge request !1926 (merged)

@balasankarc I believe it is still necessary for upgrading from gitlab-ce to gitlab-ee and vice versa on rpm installs.

Can you test that scenario on centos6 with your package?

closed

@twk3 Ah. You are correct. %postun of older package happens after %post of new package. So, it will remove the symlinks. We need posttrans.

From https://fedoraproject.org/wiki/Packaging:Scriptlets?rd=Packaging:ScriptletSnippets#Ordering

---

Ordering

The scriptlets in %pre and %post are respectively run before and after a package is installed. The scriptlets %preun and %postun are run before and after a package is uninstalled. The scriptlets %pretrans and %posttrans are run at start and end of a transaction. On upgrade, the scripts are run in the following order:

1. %pretrans of new package
2. %pre of new package
3. (package install)
4. %post of new package
5. %triggerin of other packages (set off by installing new package)
6. %triggerin of new package (if any are true)
7. %triggerun of old package (if it's set off by uninstalling the old package)
8. %triggerun of other packages (set off by uninstalling old package)
9. %preun of old package
10. (removal of old package)
11. %postun of old package
12. %triggerpostun of old package (if it's set off by uninstalling the old package)
13. %triggerpostun of other packages (if they're setu off by uninstalling the old package)
14. %posttrans of new package

---

Hence, closing this MR.