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

Update nodeinfo #446

Merged
merged 4 commits into from
Feb 17, 2019
Merged

Update nodeinfo #446

merged 4 commits into from
Feb 17, 2019

Conversation

zcdunn
Copy link
Contributor

@zcdunn zcdunn commented Feb 15, 2019

Fix #433

I added the repo link to Cargo.toml so that software.repository could be configurable like @rhaamo suggested. I don't know if it's ok to include software.repository without bumping the schema version, but I didn't know if that would break any clients that parse nodeinfo with a hardcoded schema version.

@rhaamo
Copy link

rhaamo commented Feb 15, 2019

software.repository is nodeinfo 2.1 only, you can however have 2.0 and 2.1 side by side, only the json for 2.1 will returns the repository key.

But that will introduce another thing to do, plume use https://xxx/nodeinfo so it would need to became https://xxx/nodeinfo/version or something like that.

keeping 2.0 would be good anyway for compatibility.

and for the schema version, if you add repository in a 2.0 version it will break on any parser forcing a validation, since 2.0 schema doesn't allow repository.

@zcdunn
Copy link
Contributor Author

zcdunn commented Feb 15, 2019

Ok. I'll update that. Is it ok to make it two separate endpoints, /nodeinfo/2.0 and /nodeinfo/2.1 since those are the only ones we provide or should the version be parameterized?

@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

Merging #446 into master will decrease coverage by 0.12%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
- Coverage   25.58%   25.45%   -0.13%     
==========================================
  Files          63       63              
  Lines        6254     6285      +31     
==========================================
  Hits         1600     1600              
- Misses       4654     4685      +31

@elegaanz
Copy link
Member

Thank you! For the versions, adding a parameter in the route could indeed do the job. And I'm not sure the json! macro allows it, but if possible only add the repository field if the requested version is 2.1

@elegaanz elegaanz added C: Enhancement New feature or request A: Federation Stuff related to Federation labels Feb 15, 2019
Copy link
Member

@elegaanz elegaanz left a comment

Choose a reason for hiding this comment

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

Thank you! 👍

@elegaanz elegaanz merged commit 7bac70a into Plume-org:master Feb 17, 2019
@zcdunn zcdunn deleted the update_nodeinfo_433 branch February 17, 2019 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Federation Stuff related to Federation C: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants