feat: support decimal 3.x, refresh dependencies, and unbreak CI - #58
Open
deepankar-j wants to merge 2 commits into
Open
feat: support decimal 3.x, refresh dependencies, and unbreak CI#58deepankar-j wants to merge 2 commits into
deepankar-j wants to merge 2 commits into
Conversation
Avalanche's `{:decimal, "~> 2.0"}` constraint was the only thing holding
hapi on decimal 2.x. Widen it to `~> 2.4 or ~> 3.0` so the library stops
constraining consumers in either direction, and bring the rest of the
dependency tree and toolchain along with it.
Fixes the fallout from the upgrade:
* req 0.7 removed `Req.Request.current_request_steps`. `run_request/1` now
always starts from `request.request_steps`, so the manual pipeline rewind
in the Poll and GetPartitions steps was both redundant and referencing a
deleted field. Dropped `reset_req_request/1` from both.
* Elixir 1.18+ type checking rejects `%Req.Response{response | ...}` when
`response` was only matched as a bare map. Typed the step function heads
as `%Req.Response{}`, and made `GetPartitions.error_response/1` return a
real `Req.Response` rather than a bare map.
* decimal 3 makes decimal128 limits the default, so `Decimal.new/1` raises
above 34 significant digits -- but Snowflake NUMBER supports precision
38, meaning a NUMBER(38,2) column crashed the decode step. Switched to
`Decimal.parse/2` with an explicit `max_digits: 38`, which also stops
trailing garbage ("33.04abc") from reaching `Decimal.new/1`. Added a
regression test.
* Elixir 1.20 deprecated `:preferred_cli_env` in `def project`; moved it to
a `cli/0` callback.
* Dropped `:output` from `available_req_options/0` -- the req step was
removed in req 0.6, so passing `output:` raised `unknown option`.
* Pruned castore, eflambe, and meck from mix.lock. finch 0.23 dropped
castore as a hard dependency; TLS still verifies certificates via OTP's
`:public_key.cacerts_get/0`.
* Bumped the CI Elixir/OTP matrix from 1.14/25.1, which cannot satisfy the
new requirements, to 1.20/29.
Also fixes a pre-existing typo in the integration suite: the "different a
user" token-cache test asserted `sub`/`iss` of TEST-ACCOUNT.TEST-USER2
while fetching a token for account "test-account-2". `TokenCache` was
correct; the assertion was a copy-paste that updated the user but not the
account.
BREAKING CHANGE: 422 responses now surface as `reason: :unprocessable_content`
with message "Unprocessable Content", instead of `:unprocessable_entity` /
"Unprocessable Entity". Plug 1.20 renamed 422's canonical reason atom per
RFC 9110 and `Avalanche.Error.http_status/2` derives both fields from
`Plug.Conn.Status`. Plug still accepts `:unprocessable_entity` as an input
alias, so `put_status(conn, :unprocessable_entity)` in consumers is
unaffected -- only code matching on `Avalanche.Error.reason` needs updating.
BREAKING CHANGE: minimum Elixir is now 1.17 (was 1.12).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every CI job was failing at the "Setup Elixir Project" step with:
This request has been automatically failed because it uses a
deprecated version of `actions/cache: v2`
GitHub hard-retired actions/cache v1 and v2. Because the failure hit the
composite setup action, Elixir was never installed and every subsequent
step died with `mix: command not found` (exit 127) -- which read as a
build failure but was pure workflow rot. This repo had not seen a CI run
since 2024-08, before GitHub began enforcing.
Bumps all three actions/cache@v2 (deps cache, build cache, dialyzer PLT
cache) and all five actions/checkout@v2 to v4. checkout@v2 had not yet
started hard-failing but runs on EOL Node 16, so it is the next break.
erlef/setup-beam@v1 and hgdata/semantic-release-action@v1 are left alone:
v1 is the current major for setup-beam, and the semantic-release action is
third-party and only runs on main.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi! These changes come from a fork at AHEAD-ClientApps/avalanche, offered back upstream in case they're useful. Happy to split this up, drop parts, or rework anything — please treat the choices below as proposals rather than recommendations, particularly the two marked⚠️ .
Branches cleanly off
4bfa436(v0.13.0), which is stillmain's HEAD, so there are no conflicts.Motivation
{:decimal, "~> 2.0"}was the only thing keeping a downstream app on decimal 2.x. Widening it pulled in the rest of the dependency tree, which hadn't moved since Aug 2024, and that surfaced a handful of genuine breakages.What's here
1.
ci:— unbreak CI (independently useful, no library impact)Every job currently fails at the "Setup Elixir Project" step:
GitHub hard-retired
actions/cachev1/v2. Because the failure lands inside the composite setup action, Elixir is never installed and every later step dies withmix: command not found(exit 127), which reads as a build failure but isn't. Bumps 3×actions/cache@v2and 5×actions/checkout@v2to@v4(checkout v2 isn't hard-failing yet but runs on EOL Node 16).erlef/setup-beam@v1andhgdata/semantic-release-action@v1left alone.This one stands on its own — worth taking even if you reject everything else, since
mainis currently red.2.
feat:— dependency refresh and the fixes it required{:decimal, "~> 2.0"}→{:decimal, "~> 2.4 or ~> 3.0"}(accepts both majors, so Avalanche constrains consumers in neither direction;~> 2.4is the floor becauseDecimal.parse/2's:max_digitslanded there), plus req 0.5→0.7, plug 1.16→1.20, finch 0.18→0.23, and ~30 others.Req.Request.current_request_steps.run_request/1now always starts fromrequest.request_steps, so the manual pipeline rewind inSteps.Poll/Steps.GetPartitionswas redundant and referencing a deleted field — it raised{:badkey, :current_request_steps}at runtime.reset_req_request/1from both steps.%Req.Response{response | …}whereresponsewas only matched as a bare map.%Req.Response{}. Also madeGetPartitions.error_response/1return a realReq.Responseinstead of a bare%{status: 500, body: nil}.Decimal.new/1raises above 34 significant digits — but SnowflakeNUMBERsupports precision 38, so aNUMBER(38,2)column crashed the decode step. Wasn't covered by a test.Decimal.parse/2with an explicitmax_digits: 38. This also closes a pre-existing hole: the oldFloat.parse/1guard accepted trailing garbage, so"33.04abc"reachedDecimal.new/1and raised instead of falling through toreturn_raw/3. Added a regression test.:preferred_cli_envindef project— warned on everymixinvocation.cli/0callback withpreferred_envs:.:outputstill inavailable_req_options/0, but that req step was removed in req 0.6 — since the list bypassesNimbleOptionsvalidation,Avalanche.run(…, output: …)raisedunknown option.castore,eflambe,meckorphaned inmix.lock, failing thedeps.unlock --check-unusedjob.:public_key.cacerts_get/0(checked with a live HTTPS request). Note this also removeseflambe, which only backed a commented-out flamegraph test — happy to restore it if you still want it.Also fixes a pre-existing test bug:
integration_test.exs"Private Key Token - works as expected for different a user" assertedsub/issofTEST-ACCOUNT.TEST-USER2while fetching a token for accounttest-account-2.TokenCachewas right; the assertion was a copy-paste that updated the user but not the account (dates to 76a7f46). It's one of the few integration tests needing no credentials, so it fails for anyone runningmix test.all.1. Minimum Elixir moves 1.12 → 1.17. Required by the upgraded deps, but a significant narrowing for a public library. If you'd rather keep 1.12+, that likely means holding some of these dependency bumps back.
2. The CI matrix is collapsed to a single
1.20/29pair. That suited a single-consumer fork, but it's probably wrong for you — it means the declared~> 1.17floor is never exercised. You almost certainly want something like[{1.17, 25}, {1.20, 29}]. I left it narrow rather than guess at your support policy.Release impact
The
feat:commit carries twoBREAKING CHANGE:footers (the Elixir floor, and the 422 rename below), so merging as-is will make semantic-release cut1.0.0, not0.14.0. Flagging it explicitly since that's a bigger decision than a dependency bump usually implies — squash-merging with an edited message would avoid it.The 422 change:
Plug.Conn.Status.reason_atom(422)returns:unprocessable_contentin plug 1.20 (RFC 9110 renamed it), andAvalanche.Error.http_status/2derivesreason/messagefromPlug.Conn.Status. So{:error, %Avalanche.Error{reason: :unprocessable_entity}}becomes:unprocessable_content, message"Unprocessable Content". Plug still accepts the old atom as an input alias, so consumers callingput_status(conn, :unprocessable_entity)are unaffected — only code matching onAvalanche.Error.reasonneeds updating. If you'd rather preserve the old contract, normalizing 422 insideAvalanche.Error.http_status/2is a small shim.Verification
mix qcpasses end to end on Elixir 1.20.3 / OTP 29.0.5:compile --all-warnings --warnings-as-errors— zero warnings (was 5)mix test— 90 passed (7 doctests, 83 tests), 100% coverage, satisfyingcoveralls.json'sminimum_coverage: 100format --check-formatted,credo --strict,deps.unlock --check-unused,xref graph --label compile-connected— cleanmix dialyzer—Total errors: 0Not verified: the integration suite. 8 of its 10 tests need real Snowflake credentials, which I don't have for this repo — they fail locally on DNS against the placeholder
AVALANCHE_SERVER. Since req 0.7 changed exactly the pipeline thatSteps.GetPartitionsandSteps.Polldrive,run/4 auto loads partitionsandrun/4 async query and status to get resultsdeserve a run against a live account before you merge. I'd rather say so than imply coverage I don't have.Incidentally,
dialyzer.ignore-warningsis a 0-byte file thatmix.exsnever references, and the dialyzer job cachespriv/pltswhile dialyxir writes the PLT to_build/dev/— so that cache never hits. Both harmless, both easy wins, not touched here.🤖 Generated with Claude Code