-
-
Notifications
You must be signed in to change notification settings - Fork 493
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 read-events feature in dev/staging by default #4591
Conversation
We have manually enabled this in production
So I think we need to change the code here: human-essentials/spec/rails_helper.rb Line 174 in d648cb3
Right now it only forces true if the env variable is true. I think we need to force false if it's not true. |
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 made a further change that removes this, and then event-mode github actions, altogether |
@@ -170,12 +170,6 @@ def self.capybara_tmp_path | |||
Capybara.server = :puma, { Silent: true } | |||
end | |||
|
|||
config.before(:each) do | |||
if ENV['EVENTS_READ'] == 'true' | |||
allow(Event).to receive(:read_events?).and_return(true) |
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 don't think we can do this - even though we're setting it to true in the seeds, it won't be set to true in the actual tests. If we want to always run it with true, we should just remove lines 174 and 176 and leave the rest in.
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.
Oh right ... seeds lot loaded in specs. I forgot. OK -- I'm putting that back. I think another week or two and we should remove all of the remaining feature-flag related things for Events, but we can do that separately.
This is needed just a bit longer until we completely remove the feature flag for Events
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.
@awwaiid this looks good to me - but based on the issue last week, are we sure we are ready for this? Do we want to wait a bit longer?
I'm sure! No going back! The issue last week is likely exactly the sort of issue we have without Events except without Events it is untracable and unfixable. |
I agree with Brock -- I really don't think we're going back at this point. I'm not quite ready to rip out the old stuff entirely , but we should have what developers are working with by default be what the banks are working with. |
On we go then! |
* Enable read-events feature in dev/staging by default We have manually enabled this in production * Remove more Event Sourcing separation * Minor documentation whitespace cleanup * Put back read_events check in specs This is needed just a bit longer until we completely remove the feature flag for Events
@awwaiid: Your PR |
We have manually enabled this in production.