Disable broken connections when feature flag is on #19730
Conversation
octavia-squidington-iv
added
area/api
alovew
force-pushed
the
anne/disable-connections-and-send-notif
branch
from
a650eda to
e5a7cd8
Compare
alovew
force-pushed
the
anne/disable-connections-and-send-notif
branch
from
e5a7cd8 to
0def914
Compare
alovew
force-pushed
the
anne/disable-connections-and-send-notif
branch
from
93827a4 to
6ed3996
Compare
alovew
requested review from
benmoriceau,
gosusnp,
jdpgrailsdev and
colesnodgrass
| @@ -114,7 +119,8 @@ public SchedulerHandler(final ConfigRepository configRepository, | |||
| jobPersistence, | |||
| eventRunner, | |||
| new JobConverter(workerEnvironment, logConfigs), | |||
| connectionsHandler); | |||
| connectionsHandler, | |||
| new EnvVariableFeatureFlags()); | |||
There was a problem hiding this comment.
Can we inject that instead of creating a new instance here, it will help for the migration to micronaut.
|
|
||
| return containsBreakingChange || preference == NonBreakingChangesPreference.DISABLE; |
There was a problem hiding this comment.
should this be an && instead of an ||? Otherwise we will disable all connection where the preference is NonBreakingChangesPreference.DISABLE.
We need an UTest to catch that.
alovew
force-pushed
the
anne/disable-connections-and-send-notif
branch
from
a9bc229 to
65d7d57
Compare
This reverts commit dc7a1d1.
alovew
force-pushed
the
anne/disable-connections-and-send-notif
branch
from
f1a2992 to
c5be1b0
Compare
| } else { | ||
| connectionStatus = ConnectionStatus.ACTIVE; | ||
| } | ||
| updateObject.status(connectionStatus); |
There was a problem hiding this comment.
We need a test that check that we are setting the expected status.
There was a problem hiding this comment.
I have a bunch of new SchedulerHandlerTests -- is there another case you think I should be testing?
There was a problem hiding this comment.
The status is being tested on each new test, and I also added it to existing tests
There was a problem hiding this comment.
Is there a test that check that we are not disabling the connection if the feature flag is set to false or if the preference is set to not disable?
There was a problem hiding this comment.
yes - you have to expand SchedulerHandlerTest in github to see it because I think it is collapsed due to the large number of changes to the file
There was a problem hiding this comment.
Oh my bad, I missed them because of them being collapsed 🤦
This reverts commit f07cef8.
This reverts commit f07cef8.
What
Check the feature flag in the discover read endpoint and if it is enabled, then disable connections that have breaking schema changes or where the user has specified that non-breaking changes should cause the connection to be disabled
Recommended reading order
Adding ConnectionStatus to SchemaRead object and updating ConnectionRead object:
openapi/config.yamlSchedulerHandler.java-> line 384WebBackendConnectionHandler.javaWebBackendConnectionHandlerTest.javaConnectionHelpers.java<- helpers to support test objectsSchedulerHandler logic to disable broken connections:
SchedulerHandler.java-> lines 376-382SchedulerHandlerTest.java