Skip to content
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

ui: Enable blocking queries by default #6194

Merged
merged 2 commits into from
Aug 5, 2019

Conversation

johncowen
Copy link
Contributor

This enables blocking queries by default. They can still be turned off via the settings page, and if anyone has turned off blocking queries in previous versions of the UI and have that preference stored in localStorage still, it will continue to respect that setting.

@johncowen johncowen requested a review from a team July 23, 2019 09:41
@johncowen johncowen added the theme/ui Anything related to the UI label Jul 23, 2019
Copy link
Contributor

@pearkes pearkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@@ -65,7 +65,7 @@ const createProxy = function(repo, find, settings, cache, serialize = JSON.strin
key: key,
type: BlockingEventSource,
settings: {
enabled: settings.blocking,
enabled: typeof settings.blocking === 'undefined' || settings.blocking,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If settings.block is undefined / not set, this would enable it, otherwise if it's set to something it'll respect that setting, which makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@@ -5,11 +5,6 @@ Feature: dc / list-blocking
I want to see changes if I change consul externally
Background:
Given 1 datacenter model with the value "dc-1"
And settings from yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the removal of the previous setting and the blocking test which should now pass with the default, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup exactly, I think there 'might' be a few other places in the tests where I specifically set it to on before running the tests, that will now essentially be a no-op now. I don't mind those still being in there though, I just removed this one to make sure it definitely works when you haven't set anything to on.

Since we've now turned on blocking queries by default this means that a
second GET request is made after the PUT request that we are asserting
for but before the assertion itself, this meant the assertion failed. We
double checked this by turning off blocking queries for this test using

```
And settings from yaml
---
consul:client:
  blocking: 0
---
```

which made the test pass again.

As moving forwards blocking queries will be on by default, we didn't
want to disable blocking queries for this test, so we now assert the
last PUT request specifically. This means we continue to assert that the
session has been destroyed but means we don't get into problems of
ordering of requests here
@johncowen
Copy link
Contributor Author

johncowen commented Aug 5, 2019

I noticed a test not passing here, and looked into it, there's more info in the last commit here, but will repeat here for reference/ease.


Since we've now turned on blocking queries by default this means that a
second GET request is made after the PUT request that we are asserting
for but before the assertion itself, this meant the assertion failed. We
double checked this by turning off blocking queries for this test using

And settings from yaml
---
consul:client:
  blocking: 0
---

which made the test pass again.

As, moving forwards, blocking queries will be on by default, we didn't
want to disable blocking queries for this test, so we now assert the
last PUT request specifically. This means we continue to assert that the
session has been destroyed but means we don't get into problems of
ordering of requests here

@johncowen johncowen merged commit 7f59481 into ui-staging Aug 5, 2019
@johncowen johncowen deleted the feature/ui-blocking-default-on branch August 5, 2019 15:31
johncowen added a commit that referenced this pull request Aug 22, 2019
-Enable blocking queries by default
-Change assertion to check for the last PUT request, not just any request for session destruction from a node page.

Since we've now turned on blocking queries by default this means that a
second GET request is made after the PUT request that we are asserting
for but before the assertion itself, this meant the assertion failed. We
double checked this by turning off blocking queries for this test using

```
And settings from yaml
---
consul:client:
  blocking: 0
---
```

which made the test pass again.

As moving forwards blocking queries will be on by default, we didn't
want to disable blocking queries for this test, so we now assert the
last PUT request specifically. This means we continue to assert that the
session has been destroyed but means we don't get into problems of
ordering of requests here
johncowen added a commit that referenced this pull request Aug 29, 2019
-Enable blocking queries by default
-Change assertion to check for the last PUT request, not just any request for session destruction from a node page.

Since we've now turned on blocking queries by default this means that a
second GET request is made after the PUT request that we are asserting
for but before the assertion itself, this meant the assertion failed. We
double checked this by turning off blocking queries for this test using

```
And settings from yaml
---
consul:client:
  blocking: 0
---
```

which made the test pass again.

As moving forwards blocking queries will be on by default, we didn't
want to disable blocking queries for this test, so we now assert the
last PUT request specifically. This means we continue to assert that the
session has been destroyed but means we don't get into problems of
ordering of requests here
johncowen added a commit that referenced this pull request Sep 4, 2019
-Enable blocking queries by default
-Change assertion to check for the last PUT request, not just any request for session destruction from a node page.

Since we've now turned on blocking queries by default this means that a
second GET request is made after the PUT request that we are asserting
for but before the assertion itself, this meant the assertion failed. We
double checked this by turning off blocking queries for this test using

```
And settings from yaml
---
consul:client:
  blocking: 0
---
```

which made the test pass again.

As moving forwards blocking queries will be on by default, we didn't
want to disable blocking queries for this test, so we now assert the
last PUT request specifically. This means we continue to assert that the
session has been destroyed but means we don't get into problems of
ordering of requests here
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants