#446 Update nodeinfo

Merged
zcdunn merged 4 commits from update_nodeinfo_433 into master 1 year ago
zcdunn commented 1 year ago (Migrated from github.com)

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.

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 commented 1 year ago (Migrated from github.com)
Owner

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.

`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 commented 1 year ago (Migrated from github.com)
Owner

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?

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[bot] commented 1 year ago (Migrated from github.com)
Owner

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
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/446?src=pr&el=h1) Report > Merging [#446](https://codecov.io/gh/Plume-org/Plume/pull/446?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/64d1944715a3fbcb5244f0b94d16eae3b0dd7d7e?src=pr&el=desc) will **decrease** coverage by `0.12%`. > The diff coverage is `0%`. ```diff @@ 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 commented 1 year ago (Migrated from github.com)
Owner

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

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 (Migrated from github.com) approved these changes 1 year ago
elegaanz (Migrated from github.com) left a comment

Thank you! 👍

Reviewers

Plume_migration_agent approved these changes 1 year ago
The pull request has been merged as 7bac70a483.
Sign in to join this conversation.
No Milestone
No Assignees
1 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.