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 #1513

Merged
merged 10 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,17 @@ on:
jobs:
build:
name: Build
runs-on: ubuntu-latest
runs-on: ubuntu-22.04

steps:
- uses: extractions/setup-just@v2
- uses: actions/checkout@v3
- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: 3.1
- name: Lint
run: bundle install && bundle exec rake rubocop
run: just lint
- name: Build
run: gem build stripe.gemspec
- name: 'Upload Artifact'
Expand All @@ -40,20 +41,21 @@ jobs:

test:
name: Test (${{ matrix.ruby-version }})
# this is needed because our JRuby test version isnt supported on ubuntu-24 (which is now ubuntu-latest)
# this version of jruby isn't available in the new latest (24.04) so we have to pin (or update jruby)
Copy link
Contributor

Choose a reason for hiding this comment

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

since you changed all of the runs-on, i might suggest moving some version of this comment to the top of the file so a future dev who might be tempted to upgrade the runs-as version would have the full context

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yeah, that's fine too. Also this change doesn't need to be included - we both changed the runs-on line in separate PRs, so this was my explanation for it. Yours also works though

runs-on: ubuntu-22.04
strategy:
matrix:
ruby-version: [2.3, 2.4, 2.5, 2.6, 2.7, '3.0', 3.1, 3.2, '3.3', jruby-9.4.0.0, truffleruby-head]
steps:
- uses: extractions/setup-just@v2
- uses: actions/checkout@v3
- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: ${{ matrix.ruby-version }}
- uses: stripe/openapi/actions/stripe-mock@master
- name: test
run: make ci-test
run: just test typecheck
xavdid-stripe marked this conversation as resolved.
Show resolved Hide resolved
env:
GITHUB_TOKEN: ${{ secrets.github_token }}

Expand All @@ -64,7 +66,7 @@ jobs:
startsWith(github.ref, 'refs/tags/v') &&
endsWith(github.actor, '-stripe')
needs: [build, test]
runs-on: ubuntu-latest
runs-on: ubuntu-22.04
steps:
- name: Download all workflow run artifacts
uses: actions/download-artifact@v4
Expand Down
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
15 changes: 9 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -364,19 +364,20 @@ New features and bug fixes are released on the latest major version of the Strip

[Contribution guidelines for this project](CONTRIBUTING.md)

The test suite depends on [stripe-mock], so make sure to fetch and run it from a
background terminal ([stripe-mock's README][stripe-mock] also contains
instructions for installing via Homebrew and other methods):
The test suite depends on [stripe-mock], so make sure to fetch and run it from a background terminal ([stripe-mock's README][stripe-mock] also contains instructions for installing via Homebrew and other methods):

```sh
go install github.com/stripe/stripe-mock@latest
stripe-mock
```

We use [just](https://github.com/casey/just) for common development tasks. You can install it or run the underlying commands directly (by copying them from the `justfile`). Common tasks include:

Run all tests:

```sh
bundle exec rake test
just test
# or: bundle exec rake test
```

Run a single test suite:
Expand All @@ -394,13 +395,15 @@ bundle exec ruby -Ilib/ test/stripe/util_test.rb -n /should.convert.names.to.sym
Run the linter:

```sh
bundle exec rake rubocop
just lint
# or: bundle exec rubocop
```

Update bundled CA certificates from the [Mozilla cURL release][curl]:

```sh
bundle exec rake update_certs
just update-certs
# or: bundle exec rake update_certs
```

Update the bundled [stripe-mock] by editing the version number found in
Expand Down
1 change: 1 addition & 0 deletions Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Rake::TestTask.new do |t|
t.pattern = "./test/**/*_test.rb"
end

# I think we can remove this; we'll run rubocop directly
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 the key here is the version test, since we support back to 2.3. is that easier to do here vs where you wanted to move it to?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO the version check in this block isn't relevant anymore.

We only install rubocop on >= 2.7 (in the gemfile), so when we were calling rake across all versions, it was important to only require rubocop on versions in which it was available. But CI now calls rubocop directly (not through rake), so we don't need the task here at all (in a conditional or otherwise).

Copy link
Contributor

Choose a reason for hiding this comment

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

oh interesting. agreed then it makes sense to remove it!

if RUBY_VERSION >= "2.7.0"
require "rubocop/rake_task"
RuboCop::RakeTask.new
Expand Down
41 changes: 41 additions & 0 deletions justfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
set quiet

import? '../sdk-codegen/utils.just'

_default:
just --list --unsorted

install *args:
bundle install {{ if is_dependency() == "true" {"--quiet"} else {""} }} {{ args }}

# ⭐ run all unit tests
test: install
bundle exec rake test

# check linting / formatting status of files
format-check *args: install
bundle exec rubocop {{ args }}
alias lint-check := format-check
xavdid-stripe marked this conversation as resolved.
Show resolved Hide resolved

# ⭐ check style & formatting for all files, fixing what we can
lint: (format-check "--autocorrect")

# copy of `lint` with less output
format: (format-check "--format quiet --autocorrect")

update-certs: install
bundle exec rake update_certs

# run sorbet to check type definitions
typecheck:
{{ if semver_matches(`ruby -e "puts RUBY_VERSION"`, ">=2.7") == "true" { \
Copy link
Member Author

Choose a reason for hiding this comment

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

just supports semver checks out of the box, so I got to simplify this block!

Copy link
Contributor

Choose a reason for hiding this comment

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

just to confirm, is this the "runs directly" referenced from the Rakefile? I think the same concerns I had with node/package.json applies; does having it in the Rakefile support some base level of development that someone who isnt comfortable with just can still benefit from?

Copy link
Member Author

Choose a reason for hiding this comment

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

just to confirm, is this the "runs directly" referenced from the Rakefile?

no, that's that we call bundle exec rubocop in just in CI instead of bundle exec rake rubocop in CI.

does having [rubocop] in the Rakefile ...

probably a little, but it's also a very common ruby pattern to bundle exec <tool> to run local tools.

Having it in the rakefile means a user could bundle exec rake rubocop instead of bundle exec rubocop. That would be useful if we were adding other arguments or configuration in that task, but since it's literally just calling rubocop like the user would, involving rake with rubocop at all strikes me as unnecessary abstraction.

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 comment is referencing how much clear this is compared to the same check in the Makefile, which is a bunch of extra bash code (compared to just, which supports semver checks directly)

Copy link
Contributor

Choose a reason for hiding this comment

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

that makes sense to me

"bundle exec srb tc" \
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 whole line gets executed if the expression is truthy

Copy link
Contributor

Choose a reason for hiding this comment

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

can you clarify this? im not sure what the comment is calling out

Copy link
Member Author

@xavdid-stripe xavdid-stripe Jan 16, 2025

Choose a reason for hiding this comment

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

Mostly just explaining the just syntax. If the semver_matches call above is "true", then just runs bundle exec src tc. I found it a little counter intuitive, because it looks like the expression is returning a string (which it is, but that string just becomes a line in the file). Was just adding context to the reviewer

Copy link
Contributor

Choose a reason for hiding this comment

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

ohhh. so this conditionally inserts "bundle exec srb tc" into the typecheck: step when it is run/evaluated, and thats what just executes. is that the standard way to do conditional execution in just, or is that something special we had to do here?

Copy link
Member Author

Choose a reason for hiding this comment

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

ohhh. so this conditionally inserts "bundle exec srb tc" into the typecheck: ste

exactly!

is that the standard way to do conditional execution in just

Yes, or something like:

should_sorbet := semver_matches(`ruby -e "puts RUBY_VERSION"`, ">=2.7")
typecheck:
    [ {{ should_sorbet }} == "true" ] && bundle exec srb tc

But then we shell out to ruby every time someone runs just, which has a small performance implication. Otherwise, they're equivalent!

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense to me, and it seems readable on its face.

} else { \
"echo \"Ruby version < 2.7, skipping srb tc\"" \
} }}

# called by tooling
[private]
update-version version:
echo "{{ version }}" > VERSION
perl -pi -e 's|VERSION = "[.\-\w\d]+"|VERSION = "{{ version }}"|' lib/stripe/version.rb