wwt / SwiftCurrent

A library for managing complex workflows in Swift
https://wwt.github.io/SwiftCurrent/
Apache License 2.0
307 stars 19 forks source link

Adds open PR badge to URL Checker exclusions. #169

Closed morganzellers closed 2 years ago

morganzellers commented 2 years ago

Checklist:

codecov-commenter commented 2 years ago

Codecov Report

Merging #169 (5e52e6e) into main (04c5890) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #169   +/-   ##
=======================================
  Coverage   91.28%   91.28%           
=======================================
  Files          92       92           
  Lines        2375     2375           
=======================================
  Hits         2168     2168           
  Misses        207      207           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fde2ddb...5e52e6e. Read the comment docs.

nickkaczmarek commented 2 years ago

@Tyler-Keith-Thompson @morganzellers Looking into this a little deeper I'm not sure there is much more we can do to assure the uptime of the shields beyond hosting our own server for them (which is an option).

Have we explored other options?

If we wanted to go down the server hosting route it seems like we might be able to leverage a service like netlify or vercel to handle the hosting and might even be able to do so for free. I'm not sure if that would be more headache than it's worth.

Another option might be to just statically pull in the images and reference them in the README. I'm not sure what sort of issues this would cause, but it seems like we could effectively cache these since the shields are only displaying static info as opposed to CI status or code coverage.

Are we comfortable with them being down and not validated?

I'm torn on this. I see them as a nice to have and while it would be good to know if they're unavailable, I'm not sure if having notifications that they're down without any ability to bring them back up is worth the headache. I'm kind of leaning towards the change @morganzellers made or downloading and caching them in our project.

I'm curious what y'all think? Maybe this is a longer discussion.

morganzellers commented 2 years ago

@nickkaczmarek @Tyler-Keith-Thompson I think it may be worthwhile to download (upload?) the static ones to the repo and just exclude the dynamic ones from the checker. I think hosting our own thing is overkill for the badges, but at the same time I don't want to end up with all of our badges excluded and it feels like that's where this is headed.

nickkaczmarek commented 2 years ago

@morganzellers do we need to exclude the dynamic badges? Like, are they as likely to go down?

nickkaczmarek commented 2 years ago

@nickkaczmarek @Tyler-Keith-Thompson I think it may be worthwhile to download (upload?) the static ones to the repo and just exclude the dynamic ones from the checker. I think hosting our own thing is overkill for the badges, but at the same time I don't want to end up with all of our badges excluded and it feels like that's where this is headed.

sideload? 😈

Tyler-Keith-Thompson commented 2 years ago

The probably isn't really that shields.io goes down it's that dynamic badges poll the github APIs and they get rate limited. FWIW none of them are actually static? Some are just slow-moving. Like the platforms we support is dynamically pulled from our podspec.

Okay maybe the "Swift Package Manager: Supported" one is static

nickkaczmarek commented 2 years ago

@morganzellers Going to look into if we can regex add the shield images to exclude_urls, but if that doesn't pan out I will approve and merge this.

morganzellers commented 2 years ago

@morganzellers Going to look into if we can regex add the shield images to exclude_urls, but if that doesn't pan out I will approve and merge this.

Great idea, sounds good!

morganzellers commented 2 years ago

Great find @nickkaczmarek! This is great