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

Add justfile, remove coveralls #3040

Merged
merged 13 commits into from
Jan 9, 2025
86 changes: 28 additions & 58 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ jobs:
runs-on: ubuntu-latest

steps:
- uses: extractions/setup-just@v2
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, CI only. For local purposes, I can update our shared mise config and it'll "just work". 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

famous last words... 😆
but yea, that sounds good to me!

- uses: actions/checkout@master

- name: Setup .NET
Expand All @@ -47,46 +48,15 @@ jobs:
- uses: stripe/openapi/actions/stripe-mock@master

- name: Run test suite
run: make ci-test
run: just ci-test

- name: Collect coverage
run: dotnet test --no-build -c Release -f netcoreapp3.1 src/StripeTests/StripeTests.csproj /p:CollectCoverage=true /p:CoverletOutputFormat=opencover /p:ExcludeByAttribute=CompilerGenerated

- name: Get branch name (merge)
if: github.event_name != 'pull_request'
run: echo "commitBranch=$(echo ${GITHUB_REF#refs/heads/} | tr / -)" >> $GITHUB_ENV

- name: Get branch name (pull request)
if: github.event_name == 'pull_request'
run: echo "commitBranch=$(echo ${GITHUB_HEAD_REF} | tr / -)" >> $GITHUB_ENV

- name: Send code coverage report to coveralls.io
xavdid-stripe marked this conversation as resolved.
Show resolved Hide resolved
if: env.COVERALLS_REPO_TOKEN
run: |
export ARGS="--opencover \
-i src/StripeTests/coverage.netcoreapp3.1.opencover.xml \
--repoToken $COVERALLS_REPO_TOKEN \
--useRelativePaths \
--commitId $commitId \
--commitBranch $commitBranch \
--commitAuthor $commitAuthor \
--jobId $jobId"
if [ ! -z "${pullRequestId}" ];
then
export ARGS="$ARGS \
--pullRequest $pullRequestId"
fi
dotnet tool run csmacnz.Coveralls $ARGS
env:
COVERALLS_REPO_TOKEN: ${{ secrets.COVERALLS_REPO_TOKEN }}
commitId: ${{ github.sha }}
commitAuthor: ${{ github.actor }}
jobId: ${{ github.run_id }}
pullRequestId: ${{ github.event.pull_request.number }}
# commented out because it's slow, but it's here if we want it
xavdid-stripe marked this conversation as resolved.
Show resolved Hide resolved
# - name: Verify formatting
# run: just format-check

- name: Pack
run: dotnet pack src/Stripe.net -c Release --no-build --output nuget
- name: 'Upload Artifact'
xavdid-stripe marked this conversation as resolved.
Show resolved Hide resolved
- name: "Upload Artifact"
uses: actions/upload-artifact@v4
with:
name: nuget
Expand All @@ -97,32 +67,32 @@ jobs:
if: github.event_name == 'pull_request'
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@master
- name: Setup .NET
uses: actions/setup-dotnet@v1
with:
dotnet-version: 8.0.x
- name: Run backcompat check
run: dotnet pack src/Stripe.net -p:RunBaselineCheck=true
- uses: actions/checkout@master
- name: Setup .NET
uses: actions/setup-dotnet@v1
with:
dotnet-version: 8.0.x
- name: Run backcompat check
run: dotnet pack src/Stripe.net -p:RunBaselineCheck=true

publish:
name: Publish
if: (((github.event_name == 'push') || (github.event_name == 'workflow_dispatch')) && startsWith(github.ref, 'refs/tags/v') && endsWith(github.actor, '-stripe'))
needs: [build]
runs-on: ubuntu-latest
steps:
- name: Download all workflow run artifacts
uses: actions/download-artifact@v4
with:
name: nuget
path: nuget
- name: Setup .NET
uses: actions/setup-dotnet@v1
with:
dotnet-version: 8.0.x
- name: Publish NuGet packages to NuGet
run: dotnet nuget push nuget/*.nupkg --api-key ${{ secrets.NUGET_KEY }} --source "nuget.org"
- uses: stripe/openapi/actions/notify-release@master
if: always()
with:
bot_token: ${{ secrets.SLACK_BOT_TOKEN }}
- name: Download all workflow run artifacts
uses: actions/download-artifact@v4
with:
name: nuget
path: nuget
- name: Setup .NET
uses: actions/setup-dotnet@v1
with:
dotnet-version: 8.0.x
- name: Publish NuGet packages to NuGet
run: dotnet nuget push nuget/*.nupkg --api-key ${{ secrets.NUGET_KEY }} --source "nuget.org"
- uses: stripe/openapi/actions/notify-release@master
if: always()
with:
bot_token: ${{ secrets.SLACK_BOT_TOKEN }}
2 changes: 2 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# NOTE: this file is deprecated and slated for deletion; prefer using the equivalent `just` commands.

.PHONY: update-version codegen-format test ci-test
update-version:
@echo "$(VERSION)" > VERSION
Expand Down
20 changes: 12 additions & 8 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ From within Visual Studio:

1. Open the Solution Explorer.
2. Right-click on a project within your solution.
3. Click on *Manage NuGet Packages...*
4. Click on the *Browse* tab and search for "Stripe.net".
3. Click on _Manage NuGet Packages..._
4. Click on the _Browse_ tab and search for "Stripe.net".
5. Click on the Stripe.net package, select the appropriate version in the
right-tab and click *Install*.
right-tab and click _Install_.

## Documentation

Expand All @@ -49,15 +49,15 @@ Stripe authenticates API requests using your account’s secret key, which you c

Use `StripeConfiguration.ApiKey` property to set the secret key.

``` C#
```C#
StripeConfiguration.ApiKey = "sk_test_...";
```

### Creating a resource

The `Create` method of the service class can be used to create a new resource:

``` C#
```C#
var options = new CustomerCreateOptions
{
Email = "[email protected]"
Expand All @@ -74,7 +74,7 @@ Console.WriteLine(customer.Email);

The `Retrieve` method of the service class can be used to retrieve a resource:

``` C#
```C#
var service = new CustomerService();
Customer customer = service.Get("cus_1234");

Expand Down Expand Up @@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this and the updated scripts below; everything else was prettier


Run all tests from the `src/StripeTests` directory:

```sh
dotnet test src
just test
# or: dotnet test src
```

Run some tests, filtering by name:
Expand All @@ -343,7 +346,8 @@ must be formatted before PRs are submitted, otherwise CI will fail. Run the
formatter with:

```sh
dotnet format src/Stripe.net.sln
just format
# or: dotnet format src/Stripe.net.sln
```

For any requests, bug or comments, please [open an issue][issues] or [submit a
Expand Down
39 changes: 39 additions & 0 deletions justfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
set quiet

import? '../sdk-codegen/justfile'
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, exactly! progressive enhancement, basically


_default:
just --list --unsorted

# base test command
[no-quiet]
_test no_build framework config:
dotnet test {{no_build}} {{framework}} src/StripeTests/StripeTests.csproj -c {{config}}
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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


# run tests in debug mode
[group('useful')]
xavdid-stripe marked this conversation as resolved.
Show resolved Hide resolved
test: (_test "" "-f net8.0" "Debug")

# skip build and don't specify the dotnet framework
[group('CI')]
ci-test: (_test "--no-build" "" "Release")

# format files as needed
[group('useful')]
format *options:
TargetFramework=net5.0 dotnet format src/Stripe.net/Stripe.net.csproj --severity warn {{options}}

# for backwards compatibility; ideally removed later
[private]
alias codegen-format := format

# verify, but don't modify, the project's formatting
[group('CI')]
format-check: (format "--verify-no-changes")

# called by tooling
[private]
update-version version:
echo "{{ version }}" > VERSION
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

exactly- just slightly different syntax. And yeah, we get real arguments now!

perl -pi -e 's|<Version>[.\-\d\w]+</Version>|<Version>{{ version }}</Version>|' src/Stripe.net/Stripe.net.csproj
perl -pi -e 's|Current = "[.\-\d\w]+";|Current = "{{ version }}";|' src/Stripe.net/Constants/Version.cs
Loading