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.

Sign up for GitHub

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

Jump to bottom

fix(snapshot): Cleanup notification handling for snapshots #917

Merged
merged 14 commits into from
Apr 29, 2022
Merged

fix(snapshot): Cleanup notification handling for snapshots #917

merged 14 commits into from
Apr 29, 2022

Conversation

hareetd
Copy link
Contributor

@hareetd hareetd commented Apr 29, 2022 •
edited by andrewazores
Loading

@hareetd hareetd added cleanup chore Refactor, rename, cleanup, etc. labels Apr 29, 2022
@hareetd hareetd requested a review from andrewazores April 29, 2022 18:17
@hareetd
Copy link
Contributor Author

hareetd commented Apr 29, 2022

Thoughts before I finish updating tests?

andrewazores
andrewazores reviewed Apr 29, 2022
View reviewed changes
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Looking good so far, just that one refactoring idea I commented

andrewazores
andrewazores reviewed Apr 29, 2022
View reviewed changes
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Implementation looks good.

andrewazores
andrewazores previously approved these changes Apr 29, 2022
View reviewed changes
@andrewazores andrewazores changed the title chore(snapshot): Cleanup notification handling for snapshots fix(snapshot): Cleanup notification handling for snapshots Apr 29, 2022
@andrewazores andrewazores added fix and removed cleanup chore Refactor, rename, cleanup, etc. labels Apr 29, 2022
@andrewazores andrewazores dismissed their stale review April 29, 2022 21:48

Dismissing for now - change looks good but should include corresponding web-client PR once that is merged

@github-actions
Copy link
Contributor

This PR/issue depends on:

@andrewazores andrewazores merged commit 75832b5 into cryostatio:main Apr 29, 2022
@hareetd hareetd deleted the fix-snapshot-creation-notifications branch April 29, 2022 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewazores andrewazores andrewazores approved these changes

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Snapshot creation notification handling should be delegated to the backend.
2 participants
@hareetd @andrewazores