Page MenuHomePhabricator

Move JCMapDataContent from JsonConfig to Kartographer
Open, MediumPublic

Description

Cross-repo dependencies make maintenance PITA otherwise. Or maybe not cause cross-repo dependencies will be there in any case - needs discussion.

Details

Related Gerrit Patches:

Event Timeline

MaxSem created this task.Apr 18 2017, 10:33 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 18 2017, 10:33 PM
MaxSem renamed this task from Move JCMapData content from JsonConfig to Kartographer to Move JCMapDataContent from JsonConfig to Kartographer.Apr 18 2017, 10:33 PM
Yurik added a subscriber: Yurik.Apr 19 2017, 4:10 AM

JCMapDataContent relies on both JsonConfig & Kartographer. But while it could still function properly without Kartographer, it cannot function at all without JsonConfig. This is similar to how MediaWiki api uses syntax highlighting - it makes the output nicer, but not required to exist.

debt added a subscriber: debt.

Moving off the sprint board - the Discovery team won't be able to do this work at this time.

Yurik added a comment.Oct 2 2018, 7:46 PM

See my previous comment, I don't think this task should be done at all.

MSantos added a subscriber: MSantos.EditedOct 4 2018, 2:28 PM

Recently we had CI complaining about this cross-repo dependencies and it's now blocking one patch, that's why we brought this to the RI Backlog, so we can investigate. You can see the error below:

18:44:15 Fatal error: Class undefined: Kartographer\SimpleStyleParser in /workspace/src/extensions/JsonConfig/includes/JCMapDataContent.php on line 48

Since moving JCMapDataContent from JsonConfig to Kartographer is not a good idea, would you suggest some other path to follow? To be honest I didn't get the time for further investigations on the code and I don't have an opinion yet.

This comment was removed by MSantos.
MaxSem added a comment.Oct 4 2018, 6:44 PM

A dependency can't be avoided here: either JsonConfig needs to depend on Kartographer, or vice versa. CI just needs to be aware of this dependency and always clone both extensions.

I see it you had this concern before https://gerrit.wikimedia.org/r/c/integration/config/+/352279

I am going to bring this topic again for discussion and maybe ask for this patch to be merged.

Change 464827 had a related patch set uploaded (by MSantos; owner: MSantos):
[integration/config@master] Allowing JsonConfig to be tested with Kartographer

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

MSantos added a subscriber: hashar.

@hashar what do you think about my thoughts here.

Jhernandez changed the task status from Open to Stalled.Mar 13 2019, 3:45 PM
Jhernandez triaged this task as Medium priority.
Aklapper changed the task status from Open to Stalled.Aug 1 2019, 5:51 PM

Change 464827 had a related patch set uploaded (by Jforrester; owner: MSantos):
[integration/config@master] Zuul: Run all Kartographer tests alongside each patch to JsonConfig

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

Change 464827 merged by jenkins-bot:
[integration/config@master] Zuul: Run all Kartographer tests alongside each patch to JsonConfig

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

Mentioned in SAL (#wikimedia-releng) [2020-03-23T21:43:59Z] <James_F> Zuul: Run all Kartographer tests alongside each patch to JsonConfig T163274

MSantos changed the task status from Stalled to Open.Wed, Apr 1, 3:15 PM