[Feature] Inflator should pull the license from kref when available

JonnyOThan commented 2 months ago

Currently, a .netkan file has to have a license entry rather than pulling one from kref (spacedock or github). The adder modules can usually correctly get the license when the "add to ckan" button is checked on spacedock. This issue simply proposes that logic be moved to the inflator instead.

The impact of this is fairly small, but it closes one small window for issues: If a mod author changes their license at some point, our current indexing system is unlikely to detect it. The license in the .netkan file would need to be manually updated.

HebaruSan commented 2 months ago

As noted elsewhere, the obstacle here isn't the lack of pulling the data (that code already exists), but the way the validation works; the NetkanValidator that runs on .netkan files before they're inflated performs the license validation checks:

https://github.com/KSP-CKAN/CKAN/blob/393544d60968b4aca107cf6f2a38527a5516afea/Netkan/Validators/NetkanValidator.cs#L20

... which include making sure the license is defined:

https://github.com/KSP-CKAN/CKAN/blob/393544d60968b4aca107cf6f2a38527a5516afea/Netkan/Validators/LicensesValidator.cs#L42-L45

If the licenses validator was removed from the netkan validator (and added to the ckan validator), the existing transformers would populate missing licenses as-is:

https://github.com/KSP-CKAN/CKAN/blob/393544d60968b4aca107cf6f2a38527a5516afea/Netkan/Transformers/GithubTransformer.cs#L145-L150 https://github.com/KSP-CKAN/CKAN/blob/393544d60968b4aca107cf6f2a38527a5516afea/Netkan/Transformers/SpacedockTransformer.cs#L100-L122

Note that many or most of the licenses on SpaceDock are invalid and have to be massaged to get them to match the corresponding string in the CKAN schema. The most common issue is that SD uses a space where the schema uses a -, which the SpaceDock adder fixes here:

https://github.com/KSP-CKAN/NetKAN-Infra/blob/99896de590583914c5155330391c694e92653110/netkan/netkan/spacedock_adder.py#L146C14-L146C21

HebaruSan commented 2 months ago

This is as far back as I was able to trace the licence-in-netkan requirement check; the very first commit creating metadata.t almost 10 years ago:

https://github.com/KSP-CKAN/NetKAN/blob/479eebd1063e5fec83df644cd043502c89fc1327/t/metadata.t#L23-L29

And here's where pjf explained it:

https://github.com/KSP-CKAN/CKAN/issues/346#issuecomment-63204526

Because our tests don't actually do any netkan inflation, the second can report a missing license when we'd inflate it just fine (eg: when the KerbalStuff license is a valid license string in our spec). In those cases, one can set "x_netkan_license_ok" : true to disable the check.

It sounds like they were getting a lot of user-composed netkans with license set to an invalid string (and they couldn't validate after inflation, because at that time there wasn't any inflation), and those needed to get caught, and the field was made required as well as a side effect...? I guess because the check was just a dictionary look-up, with no explicit check for absent or null:

    my $mod_license = $metadata->{license} // "(none)";

    ok(
        $metadata->{x_netkan_license_ok} || $licenses{$mod_license},
        "$shortname license ($mod_license) should match spec. Set `x_netkan_license_ok` to supress."
    );
}

Based on that, I think I can finally confidently say that there isn't a deeper reason for why this is required in the netkan, and it's safe to change it. 🎉

KSP-CKAN / CKAN

[Feature] Inflator should pull the license from kref when available #4136