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

Enable running the ruby tests with alternate redis host/port #202

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

rwstauner
Copy link
Contributor

@rwstauner rwstauner commented Apr 7, 2023

Tests pass with dev on macs and dev test works on spin (where redis is on a non-standard port and the railgun host does not exist).

@ChrisBr
Copy link
Contributor

ChrisBr commented Apr 8, 2023

Just wondering, why is this required? Because you can't inject the port?

@rwstauner
Copy link
Contributor Author

rwstauner commented Apr 8, 2023

yeah, i was trying to run the test suite on spin where redis is on a non-standard port and the railgun host doesn't work

@rwstauner rwstauner changed the title Respect redis env in ruby test suite Enable running the tests with a non-standard REDIS_PORT Apr 8, 2023
@rwstauner rwstauner changed the title Enable running the tests with a non-standard REDIS_PORT Enable running the ruby tests with a non-standard REDIS_PORT Apr 8, 2023
@rwstauner rwstauner changed the title Enable running the ruby tests with a non-standard REDIS_PORT Enable running the ruby tests with alternate host/port Apr 8, 2023
@rwstauner rwstauner changed the title Enable running the ruby tests with alternate host/port Enable running the ruby tests with alternate redis host/port Apr 8, 2023
@@ -0,0 +1,17 @@
# frozen_string_literal: true

module RedisHelper
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about introducing a new concept here. Could we not just use REDIS_URL which is $REDIS_HOST:$REDIS_PORT

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, REDIS_URL should have priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

REDIS_URL includes the database (which tends to be 0), where as the tests have been using 7.

If that isn't a concern i'm happy to switch to it.

Do we want to prefer REDIS_URL before HOST/PORT, or just use URL and drop support for HOST/PORT altogether?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, not a concern. Let's just go with REDIS_URL only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still wondering if we need this abstraction at all if it's just a ENV.fetch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's still the default string being repeated but at this point we could inline it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@rwstauner rwstauner merged commit 10d895f into master Apr 11, 2023
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems April 11, 2023 18:37 Inactive
@rwstauner rwstauner deleted the rwstauner/redis-port branch April 11, 2023 21:11
@rwstauner rwstauner self-assigned this May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants