Add justfile, remove coveralls #3040
Conversation
| @@ -24,6 +24,7 @@ jobs: | |||
| runs-on: ubuntu-latest | |||
|
|
|||
| steps: | |||
| - uses: extractions/setup-just@v2 | |||
There was a problem hiding this comment.
I had to allow this in repo settings. I gave the code a quick readthrough and it's super straightforward: it builds the name of a crate (extractions/setup-just | index.ts) and feeds it into another extension by the same author, which reads github releases to download the correct binary for the platform (extractions/setup-crate | index.ts).
It's the action recommended in the just README, so I feel good about it.
There was a problem hiding this comment.
It would be nice to not have to rely on a 1 person organization for this. is there value in us forking it and using the fork? it doesn't look super complicated so I wouldn't expect it to get updated often, and having the fork gives us some stability if something changes for them
There was a problem hiding this comment.
I'm not too worried about it, but I understand the concern. This approach takes care of supporting any platform, but given that we're only running on linux, we can probably get away with an apt-get install just (docs), which I should have thought of in the first place.
Do you feel better about that? I don't really want to be in the business of maintaining a GH action, even if it's just to check and sync our fork periodically. It just smells like maintenance we're not going to do.
There was a problem hiding this comment.
oh right because this is just for CI. i think thats reasonable, and if we end up having to support another platform (i.e. if we have to change a runner to be a macos runner for codesigning or something) its reasonable to assume we can find a way to install just thru an os-specific official channel there too.
There was a problem hiding this comment.
Yep, CI only. For local purposes, I can update our shared mise config and it'll "just work". 😁
There was a problem hiding this comment.
famous last words... 😆
but yea, that sounds good to me!
| @@ -320,10 +320,13 @@ go install github.com/stripe/stripe-mock@latest | |||
| stripe-mock | |||
| ``` | |||
|
|
|||
| Lastly, we use [just](https://github.com/casey/just) for running common development tasks. You can also read the `justfile` and run those commands directly. | |||
There was a problem hiding this comment.
I added this and the updated scripts below; everything else was prettier
| @@ -0,0 +1,29 @@ | |||
| set quiet | |||
|
|
|||
| import? '../sdk-codegen/justfile' | |||
There was a problem hiding this comment.
the trailing ? on import means to swallow the error if this file is missing. So we'll get bonus actions, but external contributors will still be able to use the recipes in this file.
There was a problem hiding this comment.
I like this. so a user that does not have codegen will still have access to repo-specific commands but perhaps not any commands that apply to or invoke the code generator?
There was a problem hiding this comment.
yep, exactly! progressive enhancement, basically
| # base test command | ||
| [no-quiet] | ||
| _test no_build framework config: | ||
| dotnet test {{no_build}} {{framework}} src/StripeTests/StripeTests.csproj -c {{config}} |
There was a problem hiding this comment.
This could be simplified if testing just always ran the build command, but I get the sense that in CI we want to build once and use that build repeatedly (for tests, packing, uploading, etc).
We could also add a just build command that was used as a pre-req for packing and uploading, but I'm not too worried about it. Since this recipe isn't called directly, I think test and ci-test are clear enough on intention.
There was a problem hiding this comment.
i think that is true re: reusing the build when packing (or previously when computing coverage). replacing dotnet build src -c Release /p:ContinuousIntegrationBuild=true with just ci-build makes sense to me and we could then assume anything with a ci- scope has to conform to certain rules (e.g. ci-build first, and then everything else is nobuild). but these are the short strokes, i think.
There was a problem hiding this comment.
let's leave it as written for now so we don't go down the rabbit hole of debugging the whole CI setup. I think these test commands are mostly set-and-forget, so i'm not worried about a little harder to read
| # called by tooling | ||
| [private] | ||
| update-version version: | ||
| echo "{{ version }}" > VERSION |
There was a problem hiding this comment.
This is the same as the makefile, but uses an action recipe argument and the just-style variable replacement ({{ varname }}). I verified that it works manually locally, though we'll have to tweak the code that calls this (the release script) once all the repos are done.
There was a problem hiding this comment.
ah, because the Makefile pulled it from an environment variable? The change makes sense to me (and I prefer an argument to an env var anyway).
There was a problem hiding this comment.
exactly- just slightly different syntax. And yeah, we get real arguments now!
|
Ok, this is ready for review @jar-stripe! I over-commented in the PR since this is the first one and I want to explain my reasoning, but the actual changes should be fairly uncontroversial. |
Why?
In an effort to modernize and simplify our local tooling, we're moving our dev commands from makefiles to justfiles. This is intended to be mostly a drop-in replacement, but some command names may change per standardization efforts.
What?
justjust(& an alternative)See Also