@brennen: Do you think this is worth a shot? (Assuming that git config --system --add safe.directory doesn't need to get puppetized or... whatever)

In T360756#9755711, @Aklapper wrote:

@brennen: Do you think this is worth a shot? (Assuming that git config --system --add safe.directory doesn't need to get puppetized or... whatever)

Setting the needed git config via Puppet is probably the best bet in the long term. There are a number of existing things that do this: https://codesearch.wmcloud.org/puppet/?q=safe.directory

Uh thanks a lot for the pointers! Let's see how far I can get copying and pasting. :P

Change #1025478 had a related patch set uploaded (by Aklapper; author: Aklapper):

[operations/puppet@production] Phabricator: Add safe.directory directives

https://gerrit.wikimedia.org/r/1025478

Consider scheduling your patch for a puppet request window if you want it to get reviewed, merged, and deployed in a more timely manner.

A patch that has no reviewers added is naturally not getting a review because there is nothing notifying people of its existence.

Please add reviewers on Gerrit that are service owners instead. Only fall back to puppet request window if there is a problem getting a reaction from them on a constant basis.

Also reviews are not happening in puppet request windows. You would have to get those separately.. but then you can also get them merged and there is no point in separately using the window to deploy them.

Plus the service owners actually would like to know about changes happening for their services.

The change is good. It's just in the wrong profile. That "phorge" profile was just used to setup a test instance of phorge. While the production class is still called phabricator. Sorry for the confusion there.

Change #1025478 merged by Dzahn:

[operations/puppet@production] Phabricator: Add safe.directory directive

https://gerrit.wikimedia.org/r/1025478

Change #1049637 had a related patch set uploaded (by Dzahn; author: Dzahn):

[operations/puppet@production] phabricator: configure git safedir for all directories

https://gerrit.wikimedia.org/r/1049637

Apologies for misunderstanding the process. Nevertheless, the fact that for certain repositories (puppet/operations stuff) there is an expectation that people get explicitly added as reviewers by the patch author or else the patch will languish forever, whereas for other repositories (mediawiki and extensions) that isn't the case is poorly communicated and has probably lead to a lot of unnecessary friction over time.

Filed that as T368492

Yea, I can confirm that adding the git config snippet will fix the "display version" issue.

But still working on how to apply the config via puppet and/or scap.

This turned into a much bigger discussion than anticipated.

Running this command manually fixes the situation while avoiding to use *.

git config --global --add safe.directory /srv/deployment/phabricator/$(readlink /srv/deployment/phabricator/deployment)

But the issue here is these entries are never cleaned up and the config would keep growing without even more code.

The same problem would happen if we run it from scap post deploy script.

Also this whole thing is a double symlink, so:

/srv/phab: symbolic link to /srv/deployment/phabricator/deployment and then:

/srv/deployment/phabricator/deployment: symbolic link to deployment-cache/revs/72ad841a0bf22b0dd1aca0d53af6c84ab044e94a

Still wondering if using just "*" is _just fine_ for this server.

Using a wild like /srv/deployment/phabricator* isn't supported by git. Only full path or global *.

There were other ideas like scap itself should add it to the config or including another file from the git config.

Apparently the latter is possible. Dumping this here:

16:14 < pepellou> never tried it, but: > You can include a config file from another by setting the special include.path (or includeIf.*.path) variable to the name of the file to be included. The variable takes 
                  a pathname as its value, and is subject to tilde expansion. These variables can be given multiple times.
16:14 < pepellou> according to https://git-scm.com/docs/git-config

In T360756#9924751, @Pppery wrote:

Apologies for misunderstanding the process.

That's not your fault. Don't worry.

Nevertheless, the fact that for certain repositories (puppet/operations stuff) there is an expectation that people get explicitly added as >reviewers by the patch author or else the patch will languish forever, whereas for other repositories (mediawiki and extensions) that isn't the >case is poorly communicated and has probably lead to a lot of unnecessary friction over time.

I am aware getting reviews is one of the biggest problems the organization has. I try to mention it whenever it makes sense since a long time. So I think you are 100% correct here.

I can't speak for mediawiki and extension repos and review practices though. (or even global operations/puppet which is kind of divided into the SRE subteams too, so depends on the service).

I can only try to do it "right" for our own services and I strongly believe that the way it should work is by adding reviewers in Gerrit who then get notified and react to the request within a reasonable time and that service owners should be involved in patches related to their services.

I am aware some users might filter mail from Gerrit (or alternatively not look at the incoming queue in web UI) but I think that's a really bad pattern. I also think that forcing any kind of realtime interaction into the review process is bad. This includes having to ping people on chats like IRC or Slack and also the puppet window.

The puppet window was introduced as a reaction to the review problem but I see it as the last resort if the normal process fails.

It has the additional problem that reviews are not supposed to happen within that window. But then, how realistic is it that one the one hand you already have a +1 and a review from someone but they ALSO don't want to merge it?

Then there is also the pattern that something is often expected to be "trivial" and is merged by people who are not usually working on this are of code and then later the actual owners are surprised by this and might have actual concerns or find out it's not as trivial as expected.

I hope this makes a little sense. I can't speak for everyone.

Filed that as T368492

Thanks for doing that. I do share frustration about the review processes.

Also, thanks for all your contributions lately.

Change #1051861 had a related patch set uploaded (by Dzahn; author: Dzahn):

[operations/puppet@production] phabricator: add script to set safedir git config for deploy repo

https://gerrit.wikimedia.org/r/1051861

Change #1051861 merged by Dzahn:

[operations/puppet@production] phabricator: add script to set safedir git config for deploy repo

https://gerrit.wikimedia.org/r/1051861

Change #1051862 had a related patch set uploaded (by Dzahn; author: Dzahn):

[operations/puppet@production] phabricator: qualify test command for 'unless' in Exec

https://gerrit.wikimedia.org/r/1051862

Change #1051862 merged by Dzahn:

[operations/puppet@production] phabricator: qualify test command for 'unless' in Exec

https://gerrit.wikimedia.org/r/1051862

Change #1051864 had a related patch set uploaded (by Dzahn; author: Dzahn):

[operations/puppet@production] phabricator: change "unless" command for generating git safedir config

https://gerrit.wikimedia.org/r/1051864

Change #1051864 merged by Dzahn:

[operations/puppet@production] phabricator: change "unless" command for generating git safedir config

https://gerrit.wikimedia.org/r/1051864

Change #1049637 abandoned by Dzahn:

[operations/puppet@production] phabricator: configure git safedir for all directories

Reason:

in favor of https://gerrit.wikimedia.org/r/c/operations/puppet/+/1051861

https://gerrit.wikimedia.org/r/1049637

@Aklapper I finally fixed this for /srv/phab/ . as in "git status" shows no errors.

But the version info is still only partial ?

Ideally the rows under "Version Information" on https://phabricator.wikimedia.org/config/ showed git commit IDs, the more complete the better. :)
But if that's not feasible we could also decline this task.

"dubious ownership in repository" was fixed before but for some reason it's broken again.. despite the script we deployed to fix this automatically :/

Mentioned in SAL (#wikimedia-operations) [2024-10-02T21:24:28Z] <mutante> phab1004 - link=$(/usr/bin/readlink -f /srv/phab) ; /usr/bin/git config -f /etc/gitconfig.d/10-phab-deploy-safedir.gitconfig --add safe.directory $link ; /bin/cat /etc/gitconfig.d/*.gitconfig > /etc/gitconfig - T360756

One notable thing is:

In the "Version Information" section on the /config/ link we see:

phorge
arcanist
wmf-ext-misc (which now?! does show version info)
ava
translations

Then when we look at phab1004:/srv/phab# cat .gitmodules we see:

submodule "arcanist"
submodule "translations"
submodule "phabricator"
submodule "tools"
submodule "libext/misc"
submodule "libext/ava"

So it roughly matches but notably "phabricator" vs "phorge" is in there.

Also:

root@phab1004:/srv/phab# git status
HEAD detached at 33a2c8d
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
  (commit or discard the untracked or modified content in submodules)
	modified:   .gitmodules
	modified:   phabricator (untracked content)

The diff is:

--- a/phabricator
+++ b/phabricator
@@ -1 +1 @@
-Subproject commit 67ec4fa3feef605378e7cd1e65b5839c3efc393e
+Subproject commit 67ec4fa3feef605378e7cd1e65b5839c3efc393e-dirty

and

 [submodule "arcanist"]
        path = arcanist
-       url = https://gitlab.wikimedia.org/repos/phabricator/arcanist.git
-       branch = wmf/stable
+       url = http://deploy2002.codfw.wmnet/phabricator/deployment/.git/modules/arcanist
 [submodule "translations"]
-       # See https://gerrit.wikimedia.org/g/translatewiki/+/bbb4cc607609ada57905ceb6804ce409684dc6d8/repoconfig.yaml
        path = translations
-       url = https://gerrit.wikimedia.org/r/phabricator/translations
-       branch = wmf/stable
+       url = http://deploy2002.codfw.wmnet/phabricator/deployment/.git/modules/translations
 [submodule "phabricator"]
        path = phabricator
-       url = https://gitlab.wikimedia.org/repos/phabricator/phabricator.git
-       branch = wmf/stable
+       url = http://deploy2002.codfw.wmnet/phabricator/deployment/.git/modules/phabricator
 [submodule "tools"]
        path = tools
-       url = https://gitlab.wikimedia.org/repos/phabricator/tools.git
-       branch = wmf/stable
+       url = http://deploy2002.codfw.wmnet/phabricator/deployment/.git/modules/tools
 [submodule "libext/misc"]
        path = libext/misc
-       url = https://gitlab.wikimedia.org/repos/phabricator/extensions.git
-       branch = wmf/stable
+       url = http://deploy2002.codfw.wmnet/phabricator/deployment/.git/modules/libext/misc
 [submodule "libext/ava"]
        path = libext/ava
-       url = https://gerrit.wikimedia.org/r/phabricator/antivandalism
-       branch = wmf/stable
+       url = http://deploy2002.codfw.wmnet/phabricator/deployment/.git/modules/libext/ava

So that part is about changing the git clone source from gitlab to deployment server for the submodules.

@brennen Can we chat about the "git status/git diff" in /srv/phab on phab1004 and the submodule situation? Maybe next office hours when we do another Phab deploy?

@brennen Can we chat about the "git status/git diff" in /srv/phab on phab1004 and the submodule situation? Maybe next office hours when we do another Phab deploy?

Sure thing.

Wow, that was fast. Thank you :)

Somehow the "dubious ownership" issue is back even though we wrote and puppetized an entire script for that :/

But ignoring that and setting global safe.directory still gets us to:

[phab1004:/srv/phab] $ git status
HEAD detached at 582cde5
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
  (commit or discard the untracked or modified content in submodules)
	modified:   .gitmodules
	modified:   phabricator (untracked content)

I tested removing the "safedir" git config and ran puppet and it added it back and it worked as designed. So I can't explain why it was broken but now it works again. Back to just the submodule status.

https://secure.phabricator.com/book/phabcontrib/article/version/ says "In general, we use git commit hashes as version identifiers, so you can identify the version of something by running git show and copy/pasting the hash from the output."

Even when fixing the git status this does not appear to fix this issue.

I'm a bit at a loss and not going to work on this soon. Un-assigning for now to reflect that. Maybe again later.