Quality of life improvements #1292
Conversation
…ding/decoding of cooke name & value
…ded a step to check that redis is running
…iner to access redis using localhost:6379
…s resolves the redis-cli not reaching the redis service
…d the redis connection verification step now that it's working
| @@ -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.
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.
Yeah looks like this behavior changed as of 1.0 crystal-lang/crystal#10485
| @@ -0,0 +1,35 @@ | |||
| on: | |||
There was a problem hiding this comment.
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.
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.
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
…he actual question, and fixed the to_h method doing too much to return a hash
|
@crimson-knight Anyway to test this locally first? I'm getting a lot of emails asking for reviews. |
|
@drujensen sorry about that! As far as the GHA go, I haven't found a way to test that workflow locally. I did take a look at the test suite and it appears within that is where we are testing the different db adapters (mostly SQLite and PG). It looks like the tests that are failing on GHA are possibly related to differences in how CI runs the tests so I may have to configure this to use a docker image to run the actual tests to simulate a local env. My local tests all pass, 480/480 now so I can merge this any time and stop email bombing you for a re-review :) |
|
@drujensen I have this at a satisfactory level for now. Github actions probably need a little more tweaking, but locally I get 480/480 tests passing, and some of these tweaks will help users on newer systems to use Amber smoothly. I'd like to get this reviewed and cut a release today if possible. |
|
@robacarp can I get your review when you get a few minutes? |
Description of the Change
amber newcommand to check which node version a user has installed and adjust the node-sass sources based on the node version foundAlternate Designs
Benefits
New Amber adopters will get a smoother experience when getting started using
amber newPossible Drawbacks
May impact pre 1.0.0 crystal users