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

PollNetMapStream: do not create any rows during long-poll operation #278

Merged
merged 5 commits into from
Jan 29, 2022

Conversation

enoperm
Copy link
Contributor

@enoperm enoperm commented Jan 16, 2022

  • read the CONTRIBUTING guidelines
  • raised a GitHub issue or discussed it on the projects chat beforehand - found existing issues.
  • added unit tests - testing requires a client, currently only available in integration tests.
  • ] added integration tests
  • updated documentation if needed - PR contains changes without changes to expected behaviour towards clients or users.
  • updated CHANGELOG.md

Fixes #232 (manually tested in the no-preauthkeys case)
Related to #93

@juanfont
Copy link
Owner

This looks good to me. @kradalby what is your opinion?

@kradalby
Copy link
Collaborator

Yep I'm happy to have this, @enoperm do you think it is suitable for merge?

@enoperm
Copy link
Contributor Author

enoperm commented Jan 28, 2022

Sorry, haven't had the time to finish up the MR the way I wanted last week. That said, I have thought a bit about it, and ultimately decided I'd rather not add a deployment test for this specific issue unless requested - the original problem looks implementation specific, and unlikely to re-emerge in the future, so long as GORM or ${insert future data storage layer} is used as intended. It sounds a bit like a waste to prolong every CI run by minutes for it.

The changes are not so large that I see it as a requirement of not breaking things, either.

@enoperm enoperm marked this pull request as ready for review January 28, 2022 20:57
@enoperm enoperm force-pushed the pollnetmap-update-only branch 3 times, most recently from 109cd5d to 2ddea63 Compare January 28, 2022 20:58
@enoperm
Copy link
Contributor Author

enoperm commented Jan 28, 2022

I seem to have messed up a bit when I first tried to merge the current main. so ended up rebase-ing and force pushing. I kindly request a re-review so as to make sure nothing weird happened that I have not noticed, but other than that, it should be okay.

kradalby
kradalby previously approved these changes Jan 29, 2022
@kradalby
Copy link
Collaborator

@enoperm Do you mind fixing the prettier lint? I have a PR open to sort the golangci one, so you can ignore that for now.

@kradalby kradalby merged commit b122d06 into juanfont:main Jan 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleted node is back online
3 participants