Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow overriding resources.remote-avc #3451

Merged
merged 2 commits into from
Jan 23, 2022

Conversation

HebaruSan
Copy link
Member

Motivation

There are a lot of mods that have remote version files but a bad URL property in the version file such that the remote version file isn't found/used. This clutters our warnings list and potentially deprives CKAN of better metadata if any of them have been updated since release:

image

Currently these can only be resolved by submitting a pull request to the mod's repo, having it merged, and then a new release being made.

Background

As of #3259, the resources.remote-avc property is set to the URL of the remote version file by the AVC transformer.

Changes

Now if the reources.remote-avc property is set upstream of the AVC transformer (i.e. in the netkan or by a previous transformer), we use that value to override the remote version file to check. This will allow us to suppress these warnings and get the most accurate compatibility data by setting the correct URL in the netkans.

@HebaruSan HebaruSan added Enhancement Easy This is easy to fix Pull request Netkan Issues affecting the netkan data labels Sep 16, 2021
@DasSkelett
Copy link
Member

While I agree that it's useful to be able to override the link, somehow it doesn't feel right to give a link under resources such an impactful meaning. For me that was always a purely informative link collection, not something that can and should actually affect other parts of the metadata (like the compatibility in this case).

As the remote file completely overrides the contents of the included file, what about allowing to specify its URL in the $vref right away, as it is the case for the $kref?

As the syntax for specifying a path inside the zip is #/ckan/ksp-avc[[/path]/avcfilename.version], we could do it as #/ckan/ksp-avc[/url/https://url.example/remote-avc.version].
Or, to simplify the parsing, as separate vref option, so like #/ckan/remote-avc/https://url.example/remote-avc.version.

In any case, it should probably be documented in the Spec.md, so it doesn't end up as a secret magic trick that only long-time contributors know (if they didn't forget about it already).

@HebaruSan
Copy link
Member Author

Unfortunately for our purposes here, $vref is not extensible; anything after ksp-avc is already defined to be used to find the internal version file, otherwise I agree that would have been the best place for this:

Netkan will first attempt to use anything after ksp-avc as a literal path within the zip file, and if that fails, will use the string as a regexp to search for a matching file to use.

And an alternative format for $vref would have a similar problem, we would still need the old format's capability to specify which file to use. Probably $vref should have taken an object value rather than a string all along so new properties could be added as needed, but it's too late now (I think?).

In fact I initially thought about using something like x_netkan_remote_avc, but then I realized we already had a field to hold what is essentially the same value, and we would just be copying the URL from one field to another. Then I considered that if I had set resources.remote-avc in my netkan or metanetkan, I think I would have expected it to be used by Netkan.

How strongly do you feel about about using another property? I'm willing to compromise, but in the meantime I'll update the spec...

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with resources.remote-avc, it doesn't make a difference in practice anyway

@DasSkelett DasSkelett merged commit 89be681 into KSP-CKAN:master Jan 23, 2022
@HebaruSan HebaruSan deleted the feature/override-remote-avc branch January 23, 2022 16:29
@HebaruSan
Copy link
Member Author

HebaruSan commented Jan 23, 2022

Documenting the command to find affected mods for future reference:

$ jq -rM 'to_entries | map(select((.value.frozen|not) and .value.last_warnings and (.value.last_warnings|contains("fetching remote version file"))) | .key) | sort | .[]' netkan.json

AllAboard
AtomicTechIncJunkyards
CanaveralPads
ConnectedLivingSpace
ContractConfigurator-ExplorationPlus
DMTanks-AeroRTG
DMTanks-CargoBays
DMTanks-Fuselage
DMTanks-SphericalTanks
DockingPortDescriptions
DraggableNavball
DuoPods
Heisenberg
Heisenberg-PlayMode-CRP
Heisenberg-PlayMode-ClassicStock
Heisenberg-PlayMode-Pristine
Heisenberg-PlayMode-Simplified
KerbalActuators
KerbalJointReinforcementNext
KerbalKomets
KerbalKomets-PlayMode-CRP
KerbalKomets-PlayMode-ClassicStock
KerbalOccupationColors
KipLowProfileHubs
Mk-33
Mk2Y
MoarKerbals
MoarKerbalsParts
MoreHitchhikers
MoreServos
NotSoSimpleConstruction
ODFC-Refueled
ODFC-Refueled-CopyPatches
ODFC-Refueled-ModifyPatches
OhScrap
ProceduralFairings-ForEverything
RealBattery
SLOTH
ServoController
Snacks
StandardPropulsionSystems
Telemagic
Timekeeper

Easy editing:

$ atom -a $(jq -rM 'to_entries | map(select((.value.frozen|not) and .value.last_warnings and (.value.last_warnings|contains("fetching remote version file"))) | .key) | sort | .[]' netkan.json | sed -e 's/$/.netkan/')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy This is easy to fix Enhancement Netkan Issues affecting the netkan data Pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants