SkynetLabs / skynet-js

A Javascript module made to simplify communication with Sia Skynet portals from the browser.
https://sdk.skynetlabs.com
MIT License
68 stars 9 forks source link

Additionally run `skynet-js` PR CI on `skynetfree` portal #509

Closed mrcnski closed 2 years ago

mrcnski commented 2 years ago

PULL REQUEST

Overview

Originally requested here:

https://github.com/SkynetLabs/skynet-js/pull/456#pullrequestreview-958770707

Didn't add siasky.net for all runs yet as it's too unstable. I had to add it for PRs from forks though, as they can't access the required repo secrets.

Checklist

linear[bot] commented 2 years ago
SKY-284 Additionally run skynet-js PR CI on skynetfree portal

[https://github.com/SkynetLabs/skynet-js/pull/456#pullrequestreview-958770707](https://github.com/SkynetLabs/skynet-js/pull/456#pullrequestreview-958770707)

mrcnski commented 2 years ago

@kwypchlo Awesome, thanks!

mrcnski commented 2 years ago

@kwypchlo Testing on more portals was requested by a few people, to find potential issues faster. e.g. if we do all our testing on a single portal, then a bug in that portal can make bad SDK code seem fine. See provided link. cc @MSevey @ro-tex

I was initially against this because it will make PRs harder to merge (the portals are not really stable honestly), and we should already be testing other portals in the daily CI. But I saw the "Run failed jobs" (recent addition I think?) feature in GitHub Actions and figured we could now easily restart any jobs that failed due to portal issues.

kwypchlo commented 2 years ago

@m-cat in that case I would refactor the github action to have matrix with portals [ siasky.net skynetfree.net skynetpro.net] and add the if condition for fork in there - if: !github.event.pull_request.head.repo.fork || matrix.portal == "siasky.net"). I think that will be simpler than copy & paste the job 3 times

mrcnski commented 2 years ago

@kwypchlo Done, although unfortunately I couldn't place the conditional on the job level. It works on the step but now CI reports that siasky.net checks succeeded which could be misleading. Not a huge deal IMO since we plan to eventually enable siasky.net (if it's ever stable enough).

MSevey commented 2 years ago

DM me when we are ready to merge this and I'll update the required Checks. I don't want to do that until we are all approved so that we don't block other PRs.