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

Merge resources and include metanetkan #2913

Merged
merged 1 commit into from
Nov 10, 2019

Conversation

HebaruSan
Copy link
Member

Motivation

In KSP-CKAN/NetKAN-Infra#70 we are saving the resources object from mod metadata into the status page's JSON objects, and in KSP-CKAN/NetKAN-status#9 these will be rendered as links on the status page for more convenient investigation of inflation errors.

Currently most $kref variants populate a property in resources (e.g. spacedock, repository, curse), except for netkan, the kref source for metanetkans. This info would be nice to have as well since metanetkans have a big impact on how issues are investigated and fixed.

Problem

Astrogator defines its resources in an "internal ckan", but all that appears in its .ckan file is "repository".

Cause

The resources property is populated with SafeAdd, which doesn't overwrite existing data on a property-by-property basis. The GitHub transformer populates resources.repository, which counts as populating the resources property, so the entire resources object of the internal ckan is dropped, even the properties that do not conflict with the one set by the GitHub transformer.

Note that this would be disruptive for the metanetkan feature suggested above, since setting resources.metanetkan before processing the remote netkan would override resources and block any downstream sources from setting values in it.

Changes

Now resources.metanetkan is added to the schema and spec and populated by the metanetkan transformer. This will allow us to click it on the status page once it is updated.

Now resources is populated with SafeMerge, a new function that effectively does SafeAdd on all of the properties of the given object. This will allow downstream transformers to add properties to resources (but they still can't change properties set by upstream transformers). This will fix mods with missing resources metadata and prevent the metanetkan resource from blocking other resources.

@HebaruSan HebaruSan added Enhancement Easy This is easy to fix Pull request Spec Issues affecting the spec Netkan Issues affecting the netkan data Schema Issues affecting the schema labels Nov 4, 2019
@DasSkelett
Copy link
Member

If we add the metanetkan resource, what about a netkan property of resources itself?
We don't need it for the status page, but I always thought about adding a link to the mod's netkan in the client (metadata tab of the GUI for example), which would be quite easy if it's included in the .ckan itself.

On the other hand, we could also calculate the url from the identifier in most cases since they are all in KSP-CKAN/NetKAN, in contrast to the metanetkans.
Just for .ckans generated from a netkan in the Backports folder a wrong link would be calculated.

@HebaruSan
Copy link
Member Author

Well, I have a use case for metanetkan now, and not netkan. I'd suggest adding the latter in the future if we find a need for it.

@HebaruSan
Copy link
Member Author

Also, the Inflator does not know the netkan. It receives the metadata in a queue message without knowing the source filename. Again, something to address if we discover a need.

@DasSkelett
Copy link
Member

Well, I have a use case for metanetkan now, and not netkan. I'd suggest adding the latter in the future if we find a need for it.

Also, the Inflator does not know the netkan. It receives the metadata in a queue message without knowing the source filename. Again, something to address if we discover a need.

Okay, am fine with that.

@DasSkelett
Copy link
Member

Just checked if we'd have to account for chained metanetkans, but the sepc doesn't allow it, so it's fine: https://github.com/KSP-CKAN/CKAN/blob/master/Spec.md#ckannetkanurl

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.

Looks good, and more important, works!

HebaruSan added a commit that referenced this pull request Nov 10, 2019
@HebaruSan HebaruSan merged commit 056d57f into KSP-CKAN:master Nov 10, 2019
@HebaruSan HebaruSan deleted the feature/netkan-merge-resources branch November 10, 2019 17:32
@DasSkelett
Copy link
Member

If my grepping is somewhat right, we have currently 149 netkans using metanetkans indexed. There will be a nice commit wave by the indexer.

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 Schema Issues affecting the schema Spec Issues affecting the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants