-
Notifications
You must be signed in to change notification settings - Fork 5
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
Move to github workflows #104
Conversation
0ba0fbc
to
58e7564
Compare
7a33dd9
to
ab30213
Compare
8356722
to
2166a02
Compare
fadc415
to
f4532c9
Compare
7a63321
to
68c4b92
Compare
68c4b92
to
4c74ed5
Compare
Gemfile
Outdated
gem 'prometheus_gcstat', git: 'git@github.com:gocardless/prometheus_gcstat_ruby' | ||
|
||
source "https://rubygems.pkg.github.com/gocardless" do | ||
gem "prometheus_gcstat", "0.1.1" |
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.
Do we need to get a new release of this gem? Not sure how old this is.
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.
Nothing got changed in the code after this version https://github.com/gocardless/prometheus_gcstat_ruby/commits/master/ except for github configs
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.
Fair. Should we allow any 0.1.x version rather than pin the exact version? That will allow for flexibility as versions upgrade.
spec/lib/que/locker_spec.rb
Outdated
before { allow(locker).to receive(:monotonic_now) { @epoch } } | ||
|
||
before do | ||
# we need this to avoid flakiness during resetting the cursor |
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.
Talked IRL, @ankithads to explain in the comment why this is needed
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.
updated
@@ -4,7 +4,7 @@ | |||
|
|||
RSpec.describe Que::Middleware::QueueCollector do | |||
subject(:collector) { described_class.new(->(_env) { nil }, options) } | |||
let(:options) { {} } | |||
let(:options) { {refresh_interval: 1.second} } |
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 is being added because we don't have database cleaner and the state is being persisted across spec runs. Lowering the interval to 1.second forces the state to be refreshed between runs.
@ankithads to change the spec to force a refresh before the flaky spec, and that is idependent of spec intervals. As a follow-up, we'll ensure that database state is cleared between examples (eg. using DatabaseCleaner).
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.
So the refresh time comes from the pg_stat_user_tables. Database cleaner does not help here.
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.
Two minor comments
Gemfile
Outdated
gem 'prometheus_gcstat', git: 'git@github.com:gocardless/prometheus_gcstat_ruby' | ||
|
||
source "https://rubygems.pkg.github.com/gocardless" do | ||
gem "prometheus_gcstat", "0.1.1" |
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.
Fair. Should we allow any 0.1.x version rather than pin the exact version? That will allow for flexibility as versions upgrade.
41162e9
to
c79a2f7
Compare
Configuring Github Actions for tests.