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

Add [ROS] version service #8169

Merged
merged 4 commits into from
Jul 12, 2022
Merged

Conversation

jtbandes
Copy link
Contributor

@jtbandes jtbandes commented Jul 6, 2022

Adds version service at /ros/v/:distro/:packageName for ROS packages. Version information is loaded directly from the ros/rosdistro repo on GitHub, using tags to determine the latest available distribution, and then parsing the distribution.yaml file from that distribution.

I was hoping to make the :distro show up as the badge label in error cases, but it seems like when an error is thrown, only the defaultBadgeLabel (or 'version') is used as the label. And I would've happily used the raw.githubusercontent.com method of loading the distribution.yaml file, but it didn't seem like one service would be able to make both GraphQL requests and REST requests (when I tried to make a REST request, it still prepended the GraphQL endpoint's base URL). Any advice on these would be appreciated!

@shields-ci
Copy link

shields-ci commented Jul 6, 2022

Messages
📖 ✨ Thanks for your contribution to Shields, @jtbandes!

Generated by 🚫 dangerJS against df1f059

@calebcartwright
Copy link
Member

Thanks for opening a PR!

To the best of my knowledge, we don't have any open issues requesting this badge with the relevant details, so before we proceed it would be helpful for us if you could speak to some of the questions we ask in the issue template for new badges so that we've a sufficient amount of context (I've not heard of ROS myself)

I think the first several are implicitly covered already, so I've taken a pass at filling them in, but certainly feel free to correct.

  • Which service is this badge for e.g: ROS (but more info/context about the nature of this service would be most appreciated)
  • What sort of information should this badge show? standard version badge
  • Is there a public API? No. The data is retrieved through a set of sequential API calls to GitHub to retrieve a yaml file which is then parsed and processed
  • Does the API requires an API key? Not in the traditional sense
  • Link to the API documentation. (anything that helps codify the schema or would otherwise be relevant)
  • What is the specific use case?
  • Who are the users of the service/target audience for the badge?

@jtbandes
Copy link
Contributor Author

jtbandes commented Jul 6, 2022

  • Which service is this badge for: ROS is an ecosystem of robotics software and tools, organized into packages. It has wide adoption across both education and industry. See ROS community metrics, ROSCon. There are two major versions of ROS (ROS 1 and ROS 2) which are actively maintained (the last distro of ROS 1 will be EOL in 2025).
  • Link to the API documentation. Some info about the rosdistro repo contents/structure is here:
  • What is the specific use case? Who are the users of the service/target audience for the badge? I'm imagining that developers of ROS packages would use the version badges in their repo READMEs just like npm package developers do with npm version badges today. The version service is segmented by distro, because multiple ros distros are being maintained at the same time (ROS 1, ROS 2), and a different package version can be released in each distro. The service supports both ROS 1 & 2 because all of the distribution info is stored in the same repository. The target audience would be developers of robotics software who are browsing available packages (e.g. on GitHub) and could use version information at a glance.

I've also shared a link to this PR in the ROS Discourse to notify the maintainers and request any feedback they might have: https://discourse.ros.org/t/shields-io-badge-service/26379

@jtbandes
Copy link
Contributor Author

jtbandes commented Jul 7, 2022

Hi @calebcartwright, just to follow up on this, is there any more info I can provide?

@calebcartwright calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label Jul 8, 2022
@calebcartwright
Copy link
Member

Hi @calebcartwright, just to follow up on this, is there any more info I can provide?

No, the info you've provided is great thanks! Someone should get around to reviewing this in the not too distant future

Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this and including tests. It is looking in pretty good shape to start with and its clear you've worked through the documentation and contributing guidelines 👍

services/ros/ros-version-helpers.js Outdated Show resolved Hide resolved
services/ros/ros-version.service.js Show resolved Hide resolved
services/ros/ros-version.service.js Show resolved Hide resolved
packageName
)

return { ...renderVersionBadge({ version }), label: distro }
Copy link
Member

Choose a reason for hiding this comment

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

For version badges, the label is usually the name of the package registry, as opposed to the package.

I think we do want to include ros in the label. With the conda badges, we include the channel in the label separated by a pipe. How about we do similar here - ros|distro e.g:

Copy link
Contributor Author

@jtbandes jtbandes Jul 11, 2022

Choose a reason for hiding this comment

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

Ok, makes sense. What do you think about including spaces around the pipe? Like this: I think it makes it more readable.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah agreed 👍 Change it on this PR and I will do another PR to update the conda one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@chris48s
Copy link
Member

chris48s commented Jul 10, 2022

I was hoping to make the :distro show up as the badge label in error cases, but it seems like when an error is thrown, only the defaultBadgeLabel (or 'version') is used as the label

This is normal - I wouldn't worry about it. We probably should set the defaultBadgeLabel to 'ros' though.

I would've happily used the raw.githubusercontent.com method of loading the distribution.yaml file, but it didn't seem like one service would be able to make both GraphQL requests and REST requests

We could find a way to do this, but tbh I think the approach you've already used of doing both queries using GraphQL is fine and maybe the neatest solution anyway. I don't think we need to change it

Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

LGTM

@repo-ranger repo-ranger bot merged commit 7cfd3f5 into badges:master Jul 12, 2022
@jtbandes jtbandes deleted the jacob/ros-version branch July 12, 2022 21:09
DenverCoder1 pushed a commit to DenverCoderOne/shields that referenced this pull request Jul 18, 2022
* Add [ROS] version service

* review feedback

* add spaces
calebcartwright added a commit that referenced this pull request Jul 22, 2022
* Add greasy fork rating badges

* refactor: combine rating classes

* refactor: remove Base from class name

* Change to a single badge with all values

* Add unit tests for GreasyForkRatingCount.render

* Average totals in color algorithm

* chore(deps): bump moment from 2.29.3 to 2.29.4 (#8170)

Bumps [moment](https://github.com/moment/moment) from 2.29.3 to 2.29.4.
- [Release notes](https://github.com/moment/moment/releases)
- [Changelog](https://github.com/moment/moment/blob/develop/CHANGELOG.md)
- [Commits](moment/moment@2.29.3...2.29.4)

---
updated-dependencies:
- dependency-name: moment
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: repo-ranger[bot] <39074581+repo-ranger[bot]@users.noreply.github.com>

* chore(deps-dev): bump @typescript-eslint/eslint-plugin (#8183)

Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 5.30.0 to 5.30.5.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.30.5/packages/eslint-plugin)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/eslint-plugin"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: repo-ranger[bot] <39074581+repo-ranger[bot]@users.noreply.github.com>

* chore(deps): bump @sentry/node from 7.4.1 to 7.5.1 (#8174)

Bumps [@sentry/node](https://github.com/getsentry/sentry-javascript) from 7.4.1 to 7.5.1.
- [Release notes](https://github.com/getsentry/sentry-javascript/releases)
- [Changelog](https://github.com/getsentry/sentry-javascript/blob/master/CHANGELOG.md)
- [Commits](getsentry/sentry-javascript@7.4.1...7.5.1)

---
updated-dependencies:
- dependency-name: "@sentry/node"
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: repo-ranger[bot] <39074581+repo-ranger[bot]@users.noreply.github.com>

* chore(deps): bump simple-icons from 7.3.0 to 7.4.0 (#8181)

Bumps [simple-icons](https://github.com/simple-icons/simple-icons) from 7.3.0 to 7.4.0.
- [Release notes](https://github.com/simple-icons/simple-icons/releases)
- [Commits](simple-icons/simple-icons@7.3.0...7.4.0)

---
updated-dependencies:
- dependency-name: simple-icons
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: repo-ranger[bot] <39074581+repo-ranger[bot]@users.noreply.github.com>

* chore(deps-dev): bump nodemon from 2.0.18 to 2.0.19 (#8179)

Bumps [nodemon](https://github.com/remy/nodemon) from 2.0.18 to 2.0.19.
- [Release notes](https://github.com/remy/nodemon/releases)
- [Commits](remy/nodemon@v2.0.18...v2.0.19)

---
updated-dependencies:
- dependency-name: nodemon
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: repo-ranger[bot] <39074581+repo-ranger[bot]@users.noreply.github.com>

* Add [ROS] version service (#8169)

* Add [ROS] version service

* review feedback

* add spaces

* add spaces round pipe in [conda] badge (#8189)

Co-authored-by: repo-ranger[bot] <39074581+repo-ranger[bot]@users.noreply.github.com>

* refactor(deps): Replace moment with dayjs (#8192)

* chore(deps): bump @sentry/node from 7.5.1 to 7.6.0 (#8197)

Bumps [@sentry/node](https://github.com/getsentry/sentry-javascript) from 7.5.1 to 7.6.0.
- [Release notes](https://github.com/getsentry/sentry-javascript/releases)
- [Changelog](https://github.com/getsentry/sentry-javascript/blob/master/CHANGELOG.md)
- [Commits](getsentry/sentry-javascript@7.5.1...7.6.0)

---
updated-dependencies:
- dependency-name: "@sentry/node"
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix missing `dayjs` -> `moment` (#8204)

Co-authored-by: Caleb Cartwright <calebcartwright@users.noreply.github.com>

* chore(deps): bump ioredis from 5.1.0 to 5.2.0 (#8201)

Bumps [ioredis](https://github.com/luin/ioredis) from 5.1.0 to 5.2.0.
- [Release notes](https://github.com/luin/ioredis/releases)
- [Changelog](https://github.com/luin/ioredis/blob/main/CHANGELOG.md)
- [Commits](redis/ioredis@v5.1.0...v5.2.0)

---
updated-dependencies:
- dependency-name: ioredis
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: repo-ranger[bot] <39074581+repo-ranger[bot]@users.noreply.github.com>

* chore(deps): bump fast-xml-parser from 4.0.8 to 4.0.9 (#8203)

Bumps [fast-xml-parser](https://github.com/NaturalIntelligence/fast-xml-parser) from 4.0.8 to 4.0.9.
- [Release notes](https://github.com/NaturalIntelligence/fast-xml-parser/releases)
- [Changelog](https://github.com/NaturalIntelligence/fast-xml-parser/blob/master/CHANGELOG.md)
- [Commits](NaturalIntelligence/fast-xml-parser@v4.0.8...v4.0.9)

---
updated-dependencies:
- dependency-name: fast-xml-parser
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* revert rebase

* Add test for all good ratings

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: repo-ranger[bot] <39074581+repo-ranger[bot]@users.noreply.github.com>
Co-authored-by: Jacob Bandes-Storch <jacob@bandes-stor.ch>
Co-authored-by: chris48s <chris48s@users.noreply.github.com>
Co-authored-by: chase <c@chse.dev>
Co-authored-by: Caleb Cartwright <calebcartwright@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants