-
Notifications
You must be signed in to change notification settings - Fork 206
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
Quality of life improvements #1292
Changes from all commits
95a6e93
7462cc1
72d9e6b
72bdd15
da256f3
16a8288
5cf7631
aa1941e
733152a
84acb1a
5ddc56e
1cb910d
5287e42
167b620
05139bb
1d52c9d
34e8536
02674c7
0cd0a53
9cac540
db6cade
78954a6
3e49484
84e1162
ab39ee1
8c82e46
29ece19
46c25f8
8345859
78b3bc3
9094d9b
7365ed4
875785d
2325ccb
10f1ced
c44e3b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Excluded: | ||
- tmp/ | ||
- myapp/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
on: | ||
push: | ||
jobs: | ||
amber-spec: | ||
|
||
runs-on: ubuntu-latest | ||
|
||
container: | ||
image: crystallang/crystal | ||
|
||
services: | ||
redis: | ||
image: redis | ||
options: >- | ||
--health-cmd "redis-cli ping" | ||
--health-interval 10s | ||
--health-timeout 5s | ||
--health-retries 5 | ||
ports: | ||
- 6379:6379 | ||
postgres: | ||
image: postgres | ||
env: | ||
POSTGRES_PASSWORD: postgres | ||
options: >- | ||
--health-cmd pg_isready | ||
--health-interval 10s | ||
--health-timeout 5s | ||
--health-retries 5 | ||
ports: | ||
- 5432:5432 | ||
|
||
steps: | ||
- name: Update & install sqlite3 | ||
run: apt update && apt install -y libsqlite3-dev redis | ||
|
||
- name: Download source | ||
uses: actions/checkout@v3 | ||
|
||
- name: Install shards | ||
run: shards install | ||
|
||
- name: Run tests | ||
run: bin/amber_spec | ||
env: | ||
REDIS_URL: "redis://redis:6379" |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,9 @@ module Amber::Router::Cookies | |
expires : Time? = nil, domain : String? = nil, | ||
secure : Bool = false, http_only : Bool = false, | ||
extension : String? = nil) | ||
name = URI.encode_www_form(name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a breaking change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No this is preserving the existing expected behavior. I had to check the std lib, and 2 years ago the behavior for setting cookie names changed from implicitly encoding names and values to not encoding them at all. So I added this to preserve the expected behavior. Now that I've said that I need to check and see if this means Amber will only work as expected on Crystal 1.0+ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah looks like this behavior changed as of 1.0 crystal-lang/crystal#10485 |
||
value = URI.encode_www_form(value) | ||
|
||
if @cookies[name]? != value || expires | ||
@cookies[name] = value | ||
@set_cookies[name] = HTTP::Cookie.new(name, value, path, expires, domain, secure, http_only, extension) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
module Amber | ||
VERSION = "1.2.2" | ||
VERSION = "1.3.0" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be running all the different combinations of tests. I thought we ran against postgres, mysql and sqlite. Also, I thought we tested different templating options using docker compose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into expanding the combination of tests more. I'm not very familiar with GHA and I'm just trying to make it work to start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And FYI I still have 10 failing tests to fix, so I won't merge this until at least everything is green on my repo