-
Notifications
You must be signed in to change notification settings - Fork 40
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
fix: Add inspection of values for ENV flags #900
Conversation
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.
Thanks soooooo much for this!
tests/integration/utils.go
Outdated
databaseDir = databaseDirValue | ||
} | ||
|
||
mapStore = true // default to 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.
todo: This is a behaviour change, as now if you only provide a badgerFile param, you will also get badger IM and map running too. This wasn't desirable before, and I doubt it is now.
If you are in faviour of the behaviour change, would recommend checking in with the rest of the team on discord first (I prefer the original, but pretty much never use it)
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.
didn't we want to remove the map store 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.
Not yet, this PR doesnt drop support for it. The original comment also applies to badgerInMemory
so we cant really just shrung this one off and assume/hope we'll drop support anyway
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.
ah right there's even an issue about removing testing using map store #683
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.
You're right this was indeed a behaviour change. I had misunderstood the impact of that if
statement. Should be fixed now.
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.
Thanks, approved :)
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.
You appear to have changed the behaviour of the defaults - strongly suggest checking in with the rest of the team first
Codecov Report
@@ Coverage Diff @@
## develop #900 +/- ##
===========================================
+ Coverage 59.68% 59.69% +0.01%
===========================================
Files 156 156
Lines 17306 17306
===========================================
+ Hits 10329 10331 +2
+ Misses 6042 6041 -1
+ Partials 935 934 -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.
LGTM, cheers for sorting it out
Relevant issue(s) Resolves sourcenetwork#684 Description This PR improved the handling of ENV flags for our integration tests.
Relevant issue(s)
Resolves #684
Description
This PR improved the handling of ENV flags for our integration tests.
Tasks
How has this been tested?
integration tests
Specify the platform(s) on which this was tested: