[Bug] Conflicts between the new bot and the web hooks?

[HebaruSan]

This file was just generated in response to KSP-CKAN/NetKAN#7391 :

https://github.com/KSP-CKAN/CKAN-meta/blob/bcc0624e44f4cc1aa7344ab05fd81a83d172fe5f/CommunityDeltaVMaps/CommunityDeltaVMaps-v2.6.0.ckan

However, it's not in the master HEAD folder:

https://github.com/KSP-CKAN/CKAN-meta/tree/master/CommunityDeltaVMaps

At first I thought this might be a glitch with GitHub, but if I refresh the CKAN GUI, the new module doesn't show up, so it looks like a real issue.

[techman83]

I'll have a look closer when I get to work, the hook could have been missed? I'm not sure I'm fully understanding what you're saying.

None of the code has changed compared to the old hooks, but we are using a much newer version of Perl.

[HebaruSan]

The hook definitely fired, there's a commit with the new file. But that file is missing from the master branch. It looks like the merge commit deleted it? I don't understand what happened either.

[HebaruSan]

Same thing just happened to KSP-CKAN/NetKAN#7392. New .ckan file:

https://github.com/KSP-CKAN/CKAN-meta/blob/8f17ba018c7800678e32646f85a8c52a8b37d915/CrewLight/CrewLight-1.19.1.ckan

However, that's showing a specific commit, and if you use the branch dropdown to switch to master, it's not there it wasn't there when I first wrote that:

https://github.com/KSP-CKAN/CKAN-meta/blob/master/CrewLight/CrewLight-1.19.1.ckan

@linuxgurugamer, FYI, some things may be a bit broken at the moment, and changes made by your PRs may not be reflected immediately in CKAN as they were before. We're looking into it, but hopefully it should only be a delay of a few hours as I expect the normal bot process to pick up the same changes...

[techman83]

I don't know that this is a new problem and part of my reasoning for the rewrite. If the indexer blows up now, it bails out and ECS restarts it. On restart it's a new container and pulls everything down, picking up where it left off.

On branch master
Your branch and 'origin/master' have diverged,
and have 1 and 1 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)

You have unmerged paths.
  (fix conflicts and run "git commit")
  (use "git merge --abort" to abort the merge)

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)

	deleted by them: CrewLight/CrewLight-1.19.1.ckan

no changes added to commit (use "git add" and/or "git commit -a")

I'm not entirely sure how it got in this state, but I don't think it's worth investing any time in figuring it out. We'd be better off investing in #15 and it's not something we have to do all at once, as we can route different paths to different containers using nginx.

Personally I think starting with Spacedocks existing endpoint is probably a good place. It's very simple and will get the process fleshed out. I'm talking like for like here, we can add a new endpoint pretty easily after.

Anyway, I'm going to close this. It's an edge case thing and having to fix it manually will motivate me to learn flask!

[HebaruSan]

A few more examples of this in the recent log. You can tell where each commit came from because the Perl code puts an extra set of quote marks around the commit message. In each case, the extra-quotes commit from Perl comes first, followed by a merge commit that does nothing, and later the new bot creates a real commit:

https://github.com/KSP-CKAN/CKAN-meta/commits/master

AntennaHelper:

CEDA-AeronautisDivision:

[HebaruSan]

It doesn't appear to be an edge case, it appears to affect every instance of the web hook.

[techman83]

Hrm. It isn't affecting the end result. But it's like the new indexer isn't seeing the new file, committing it, and it is designed to be the source of the truth, so any conflicts it'll win.

Now the indexer is supposed to pull on every batch (using the context manager), but maybe that's not happening? But on commit it definitely happens and then wins.

The only immediate danger is extra commits, it's not breaking consistency. So I'll take a look after work.

[techman83]

So. I know why this is occurring

This is what a regular git clone config looks like:

[core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
[remote "origin"]
	url = /tmp/tmpnvd6f81d/upstream
	fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
	remote = origin
	merge = refs/heads/master

This is what ours looks like

[core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
[remote "origin"]
	url = /tmp/tmpnvd6f81d/upstream
	fetch = +refs/heads/*:refs/remotes/origin/*

So our local master isn't tracking to origin, so the pull doesn't pull anything. I'm honestly very close to setting it on fire and just calling a subprocess to get this blasted thing to clone properly. It really shouldn't be this hard to initialise a repo.

[HebaruSan]

Hmm, I notice that init_repo doesn't actually have a clone or clone_from call, just mkdir, init, create_remote, and pull. Maybe that's part of the problem?

[techman83]

I'm wondering if the config was a read herring or not, as the repo tracking appears to be the same and the config is definitely now correct.

[core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
[remote "origin"]
	url = [email protected]:KSP-CKAN/CKAN-meta.git
	fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
	remote = origin
	merge = refs/heads/master

#36 is still an issue and hasn't updated since the new bot was started. I'm not 100% convinced the pull from origin is doing what it is supposed to be doing.

[HebaruSan]

Yeah, it's really bizarre. Do you know where these merge commits are coming from? They should not be necessary since it's just pulling on a clean working dir, committing to master, and then pushing again; there's no need to merge anything in that flow, yet apparently it's doing it:

Maybe we should try it without strategy='ours'?

[techman83]

Hmm, that's only the pull and should only trigger if there is conflict. We've been using that strategy for as long as I can remember. I think we need to do some testing, it's pretty easy, so it should be easy enough to replicate.

That class can be pulled into ipython to do commits and updates using the same methods. So it should be relatively straight forward to figure out, I just need to set aside some time to do it (still in catch up mode)

[HebaruSan]

https://git-scm.com/docs/git-pull

ours
This option forces conflicting hunks to be auto-resolved cleanly by favoring our version. Changes from the other tree that do not conflict with our side are reflected to the merge result. For a binary file, the entire contents are taken from our side.

This should not be confused with the ours merge strategy, which does not even look at what the other tree contains at all. It discards everything the other tree did, declaring our history contains all that happened in it.

!!!!! That's exactly what's happening! The Indexer is discarding everything the other trees (web hooks, download counter) did in its merge commits!

ours
This resolves any number of heads, but the resulting tree of the merge is always that of the current branch head, effectively ignoring all changes from all other branches. It is meant to be used to supersede old development history of side branches. Note that this is different from the -Xours option to the recursive merge strategy.

strategy='ours' is definitely the problem. I think we instead wanted the ours option to the recursive strategy. Pull request inbound once I figure out what that syntax looks like in python.

NetKAN-Infra/netkan/netkan/indexer.py

Line 100 in e5735d3

self.mod_version, strategy='ours'

We've been using that strategy for as long as I can remember.

No, we used -X ours before, and now we're using -s ours.

https://github.com/KSP-CKAN/NetKAN-bot/blob/4f218a2085f9a5e49f151c499a9192e97c5af728/lib/App/KSP_CKAN/Tools/Git.pm#L411

[HebaruSan]

This is also blocking manual changes to CKAN-meta!

@DasSkelett tried to delete this file:

https://github.com/KSP-CKAN/CKAN-meta/blob/master/Decalc-o-mania/Decalc-o-mania-1.2.ckan

... in this commit:

KSP-CKAN/CKAN-meta@0b681f2

However, the bot brought it back thirteen minutes later in a merge commit, and there it remains.

The reason that the Indexer seems to always "win" these conflicts is that it pulls and pushes once per group of 10 modules while it's running. The old bot did one big push at the end (if I'm not mistaken). This change is fine in principle, but at the moment it means that we have a nuisance process reverting everyone else's changes every few minutes during certain periods.

[techman83]

Yeah, the old bot did one at the end of the run, unless there was a staged commit. This bot aimed to get committed data out the door as soon as it was available. This bug however was quite problematic!