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

[GithubRelease],[GithubTag] filter by prefix #8205

Closed
wants to merge 12 commits into from
Closed

[GithubRelease],[GithubTag] filter by prefix #8205

wants to merge 12 commits into from

Conversation

mvndaai
Copy link

@mvndaai mvndaai commented Jul 16, 2022

This change allows:

@shields-ci
Copy link

shields-ci commented Jul 16, 2022

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

Generated by 🚫 dangerJS against 6e21b06

@calebcartwright calebcartwright added the service-badge New or updated service badge label Jul 16, 2022
@calebcartwright
Copy link
Member

Thanks for the PR @mvndaai!

One possible complication/bug in the code, especially for the tests, is that it requests the most recent 100 tags so if a prefix is further than 100 tags away it will ignore the prefix.

This is true but unrelated to the addition of this PR, and more just another impact of the fact that we don't support walking the pages for releases/tags yet (refs #6766); I wouldn't worry about this

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Thanks again, few items left inline for your review

services/github/github-release.tester.js Outdated Show resolved Hide resolved
services/github/github-tag.service.js Outdated Show resolved Hide resolved
services/github/github-tag.tester.js Outdated Show resolved Hide resolved
@shields-cd shields-cd temporarily deployed to shields-github-version--j0m2ov July 22, 2022 03:08 Inactive
@mvndaai mvndaai changed the title [GithubRelease] and [GithubTag] filter by prefix [GithubTag] filter by prefix Jul 27, 2022
@mvndaai mvndaai changed the title [GithubTag] filter by prefix [GithubRelease],[GithubTag] filter by prefix; Github tag filter by subpackage Jul 29, 2022
@mvndaai mvndaai requested a review from chris48s August 2, 2022 19:39
services/github/github-tag.service.js Outdated Show resolved Hide resolved
services/github/github-common-release.js Outdated Show resolved Hide resolved
services/github/github-tag.service.js Outdated Show resolved Hide resolved
services/github/github-tag.tester.js Outdated Show resolved Hide resolved
@mvndaai mvndaai requested a review from chris48s August 4, 2022 04:54
@mvndaai mvndaai changed the title [GithubRelease],[GithubTag] filter by prefix; Github tag filter by subpackage [GithubRelease],[GithubTag] filter by prefix Aug 7, 2022
@@ -28,6 +28,10 @@ t.create('Tag (repo not found)')
.get('/v/tag/badges/helmets.json')
.expectBadge({ label: 'tag', message: 'repo not found' })

t.create('Tag (filter by prefix + trim prefix at slash)')
.get('/v/tag/ros/rosdistro.json?subpackage=galactic/&trimPrefixAtSlash')
Copy link
Member

Choose a reason for hiding this comment

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

As I was reading through the diff again I was starting to feel like I'd be more comfortable with a greater number of tests, and I definitely feel that way after getting to this test, which is using the no-longer valid/existent query parameter name.

I think it would be helpful to try to move more of the logic out of the handle function(s) (which are more difficult to test) into standalone functions (either class members or free functions) that can be directly unit tested and consumed by handle and/or fetch functions as needed

Copy link
Author

@mvndaai mvndaai Aug 26, 2022

Choose a reason for hiding this comment

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

I fixed that test. I had assumed that Joi.date().required() meant that the string had to be only a date, not just include one. Since all that repo's tags are something like galactic/2022-10-04 it matched on all tags, which is disappointing because originally I had made a regex to just match the date but switched to Joi.date instead on feedback. Now It checks for a date and a prefix of 20.

I just reviewed the code and I don't know how much more I can break up. I could maybe make a function of splitting the prefix when you want to trim but that doesn't feel complex enough to need a function. If you have some pointers I would gladly take them.

Thanks for all the time you have spent reviewing this!

@mvndaai mvndaai requested review from calebcartwright and chris48s and removed request for chris48s and calebcartwright August 26, 2022 22:49
@mvndaai mvndaai requested review from calebcartwright and removed request for chris48s August 26, 2022 22:49
@github-actions
Copy link
Contributor

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

Generated by 🚫 dangerJS against 828833a

@chris48s
Copy link
Member

chris48s commented Jul 3, 2023

Hello. We have just shipped a new feature which allows you to pass an arbitrary filter to the github tag and release badges. For example

The filter param can be used to apply a filter to the project's tag or release names before selecting the latest from the list. Two constructs are available: * is a wildcard matching zero or more characters, and if the pattern starts with a !, the whole pattern is negated.

e.g:
https://img.shields.io/github/v/tag/badges/shields
https://img.shields.io/github/v/tag/badges/shields?filter=%21server-%2A

This feature should be powerful and flexible enough to enable this use-case (and a bunch of others).

@mvndaai mvndaai deleted the github_version_prefix branch July 6, 2023 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants