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

[OSSLifecycle OSSLifecycleRedirect] Add file_url param to pull from non-github sources #10489

Merged
merged 5 commits into from
Sep 5, 2024

Conversation

JoeIzzard
Copy link
Contributor

This PR addresses #10244 (OSS Lifecycle GitLab) by adding a queryParam of file_url. When this parameter is provided, we fetch the provided URL instead of defaulting to the GitHub RAW Content URL. Existing badges using the {user}/{repo} and {user}/{repo}/{branch} pattern are maintained, however providing file_url with these will still override the URL we fetch.

UI Changes:

  • Added a OSS Lifecycle (file) OpenAPI entry for the new end point without path parameters
  • Changed OSS Lifecycle to OSS Lifecycle (GitHub)
  • Changed OSS Lifecycle (branch) to OSS Lifecycle (GitHub branch)

Changes to tests:

  • Added new test to test using file_url against the Netflix OSS Tracker Repo

One potential improvement would be to rename the service class, currently it is OssTracker however everywhere else is referred to as osslifecycle.

Add file_url param to allow pulling from non-github sources
Add test using file_url variant of OSS Lifecycle badge using Netflix OSSTracker repo
Copy link
Contributor

github-actions bot commented Sep 2, 2024

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

Generated by 🚫 dangerJS against a2c2750

@chris48s
Copy link
Member

chris48s commented Sep 2, 2024

I think the way I'd prefer to do this would be to make the badge route

/osslifecycle?file_url=

(i.e: completely get rid of :user, :repo and :branch params on the OssTracker class) and only implement fetching from an arbitrary URL on the OssTracker class.

Then we implement the legacy compatibility with redirectors that catch /osslifecycle/:user/:repo or /osslifecycle/:user/:repo/:branch+ and issue an appropriate redirect to /osslifecycle?file_url=https://raw.githubusercontent.com/whatever... That would be in line with how we usually do this. There are some similar examples you could look at:

Really the key thing is using the transformQueryParams method.

@chris48s chris48s added the service-badge Accepted and actionable changes, features, and bugs label Sep 2, 2024
Remove all references and code for the old /{user}/{repo} and /{user}/{repo}/{branch} functionality.
This will be replaced by a redirect service.

Also updated all tests to use the new file_url method
add a redirector for the original osslifecycle pattern
@JoeIzzard JoeIzzard changed the title [OSSLifecycle] Add file_url param to pull from non-github sources [OSSLifecycle OSSLifecycleRedirect] Add file_url param to pull from non-github sources Sep 2, 2024
@JoeIzzard
Copy link
Contributor Author

I didn't realise there was a redirector method, makes sense to do it that way.

I have stripped back the osstracker to just include fetching from any URL with file_url. All the existing checks are now using the file_url method so was able to remove the test I added in my first version. Added a redirector to handle the old methods, and added two tests to test with and without the branch parameter.

Copy link
Contributor

github-actions bot commented Sep 3, 2024

🚀 Updated review app: https://pr-10489-badges-shields.fly.dev

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. Couple of minor changes then this is good to merge

services/osslifecycle/osslifecycle.service.js Show resolved Hide resolved
services/osslifecycle/osslifecycle.service.js Outdated Show resolved Hide resolved
Tweak the Description to remove reference to GitHub as now works with any repository,
and updated handle function to remove reference to unused variables.
@chris48s chris48s added this pull request to the merge queue Sep 5, 2024
Merged via the queue into badges:master with commit 9717fc2 Sep 5, 2024
23 checks passed
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.

2 participants