Improve deprecation of git_data_dirs

In https://gitlab.com/gitlab-com/infrastructure/issues/2740, the git_data_dirs format changed and broke dev. It was difficult to understand what was broken and how things needed to be fixed.

I think we deprecated this in b9144d23, but I think this is going to make upgrading to 10.0 a bit challenging for GitLab.com. I also worry we'll have customers also breaking left and right. Could we:

1. Improve the error message with a link to an explanation

I see that we had a warning message before, but I don't think any of us saw it. Should we consider preserving backwards compatibility but put a warning at the end of the reconfigure?

/cc: @twk3, @lbot

changed milestone to %10.0

assigned to @twk3

@stanhu yes, we can link to our docs for the error message

Until we get some code in for: https://gitlab.com/gitlab-org/omnibus-gitlab/issues/2533 I'm not sure that we know how to make deprecation warnings easier to see.

The switch away from git_data_dir is over a year old, but the change in format for git_data_dirs happened for 9.0. I think we could drop git_data_dir now in GitLab 10, and leave the git_data_dirs format change the way it was until GitLab 11. (With us hopefully getting the chance to improve where the deprecation messages are shown within the next few releases.)

@twk3 @stanhu if we don't end up getting in #2533 I think we have to push git_data_dir as well. It seems to be everywhere in my testing of 10.0. Our helm charts use the old git_data_dir style, so does DC/OS, and so I imagine a lot of Omnibus people do as well. (Like ourselves)

Given that this is the 13th and we are 9 days away from release, my confidence is not high on #2533 since we don't even have an MR going for it yet. I know @marin felt strongly about this, but I feel like without #2533 this will be too disruptive.

I would suggest we:

• Revert this change for %10.0.
• Try hard to get #2533 in %10.1, so we can start warning people about these deprecations and get them prepared.
• Deprecate both again in %11.0.

@joshlambert I would be in favor of that approach, but I can see why @marin wanted to make this a hard change in 10.0. I think we really need to call out this change in the blog post/upgrade guide because I didn't see any warnings to this effect.

@stanhu it was noted in the 9.5 blog post: https://about.gitlab.com/2017/08/22/gitlab-9-5-released/#drop-support-of-legacy-git-storage-configuration

And we would do the same of course in 10.0. I worry though that without #2533 getting in, and relying solely on the blog post, is not enough.

Ah, thanks. Given that we didn't even know the problem existed on dev, I would say that others will be bit just as hard.

@twk3 your thoughts? What is your confidence on #2533?

We just hit this error with the canary git01 node on GitLab.com.

As far as I know, nobody is actively working on https://gitlab.com/gitlab-org/omnibus-gitlab/issues/2533, at least while Balu has been away.

I will revert the change, because it breaks the current install. If I can get in a change before release that instead breaks during preinst, I would still be in favour of doing that. This would result in an install failure, but no downtime introduced as you fix the problem. Similar to what we are doing for pg 9.2.

mentioned in merge request !1925 (merged)

@twk3 that sounds like a good idea. We'd also get the chance to provide an actual failure reason too that was more readable?

@joshlambert I'm going to try and make it more readable, but it won't be more visible, as the what will happen is the postinstall message of their previous install will re-run, and spit out some additional text.

mentioned in merge request !1937 (merged)

Checking in the preinst template turned out to be a little more flakey and difficult than I would like. So I think we will keep supporting git_data_dir for GitLab 10.0 (@joshlambert the release post might need updating?)

For 10.0 I am going to slightly improve the deprecated warning output from previous with this MR: https://gitlab.com/gitlab-org/omnibus-gitlab/merge_requests/1937

Then we should followup soon with https://gitlab.com/gitlab-org/omnibus-gitlab/issues/2533 and https://gitlab.com/gitlab-org/omnibus-gitlab/issues/2782

And then we can remove the support in GitLab 11.

@twk3 I never added it to the release post for 10.0, so I think we are good there.

Sounds good on the changes to better highlight the deprecation.

closed via merge request !1937 (merged)