Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[config] Remove deprecated configuration fields #2771
[config] Remove deprecated configuration fields #2771
Changes from 37 commits
4fd07bb
3792ac3
c996f46
63f34ac
7d5f0f8
a7bfc03
be9f380
05d2923
c88d677
8151c0f
437291a
2e3963f
7dd5233
4b2cdc1
cd7cf2a
6bf8f67
5516cb0
5f28d5d
69efb04
2b7fbf5
dd879b2
c6be048
8d057a1
448ae35
0c6f74a
20fb0b8
cd8e974
e77ce68
6396cd8
17cd9a0
fe3eca7
1d79f40
9909bb5
3ed1002
f15e43e
6c01aea
215eca4
ebeef92
cb04ded
8490406
8be56c3
21ca72f
963da12
8b1dc1d
7c0c897
9d93823
361f7ed
700cfd0
03e2235
99f95ea
0233987
fad7a04
3a27236
4fd1f30
95f93df
3a4e3b9
e7de3f1
0b42cec
b07bc54
f3474f1
df4fcb5
cf7b4da
20ab22d
1e63cf9
e3f65e9
9436afc
25fdee3
256cd58
7eee1a0
2356155
56261eb
88efec4
efae023
0fc5de1
47720e3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
If we want to preserve this behavior, maybe we add an
Enabled
flag to each bootstrap type (e.g. underpeers
,commitlog
, etc.)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.
Hmm, the tests passed w/o the peers bootstrapper disabled? I think it's prob fine if the docker integration tests passed.
For integration tests we explicitly configure the bootstrappers we need.
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.
Yeah so all the integration tests passed with just the standard ones included (including peers). So I assume it is fine? I'm not sure what the original reason was for excluding them given things are passing.
However, since we removed the
bootstrappers
list from the config, we can no longer select a subset of bootstrappers to run for specific integration tests. So I think if we do end up needing that flexibility, we can just add in an Enabled or Disabled flag to each of the types under thebootstrap
config section. Does that make sense?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.
If we can do it in integration tests, we can make it a code hook only rather than a config hook if we do end up needing to disable bootstrappers?
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.
Seems like the integration tests all work w/ standard bootstrappers enabled so I'd vote we keep everything as-is since it is more comprehensive testing / less special casing
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.
+1 to less special casing.