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

fix: Only check for events queue on subscription request #1326

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #1189

Description

Only checks for the events queue on subscription request. This makes the optional WithUpdateEvents database (constructor) option optional again (it would error without it for any kind of non-introspection request).

Tested by running a few of the integration tests having hacked away the WithUpdateEvents db option in utils2.go, also checked that without the fix the hacks would result in an error.

As the integration test framework does not allow for db configuration atm, and we are planning on releasing today I am skimping on adding proper automated tests for this right now.

Tested by running a few of the integration tests having hacked away the WithUpdateEvents db option in utils2.go.
@AndrewSisley AndrewSisley added bug Something isn't working area/db-system Related to the core system related components of the DB action/no-benchmark Skips the action that runs the benchmark. labels Apr 11, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.5 milestone Apr 11, 2023
@AndrewSisley AndrewSisley requested a review from a team April 11, 2023 16:58
@AndrewSisley AndrewSisley self-assigned this Apr 11, 2023
@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #1326 (2db6cc3) into develop (61ff28e) will decrease coverage by 0.05%.
The diff coverage is 72.72%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1326      +/-   ##
===========================================
- Coverage    70.55%   70.51%   -0.05%     
===========================================
  Files          184      184              
  Lines        17784    17786       +2     
===========================================
- Hits         12548    12541       -7     
- Misses        4284     4290       +6     
- Partials       952      955       +3     
Impacted Files Coverage Δ
db/subscriptions.go 75.67% <72.72%> (+1.38%) ⬆️

... and 2 files with indirect coverage changes

Earlier return makes the code more readable IMO
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

LGTM

@AndrewSisley AndrewSisley merged commit 3b0c55b into develop Apr 11, 2023
@AndrewSisley AndrewSisley deleted the sisley/fix/I1189-with-update-events branch April 11, 2023 17:16
shahzadlone pushed a commit that referenced this pull request Apr 13, 2023
* Only check for events queue if subscr. req

Tested by running a few of the integration tests having hacked away the WithUpdateEvents db option in utils2.go.
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…rk#1326)

* Only check for events queue if subscr. req

Tested by running a few of the integration tests having hacked away the WithUpdateEvents db option in utils2.go.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/db-system Related to the core system related components of the DB bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

db.db requires optional WithUpdateEvents option
2 participants