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

feat: option to disable writeCache and fix leak in subscriptions #1622

Merged
merged 8 commits into from
Jul 3, 2023

Conversation

robertsLando
Copy link
Member

Move commits from master

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (dcb24de) 86.25% compared to head (f02cff8) 86.28%.

❗ Current head f02cff8 differs from pull request most recent head ef31a76. Consider uploading reports for the commit ef31a76 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1622      +/-   ##
==========================================
+ Coverage   86.25%   86.28%   +0.03%     
==========================================
  Files          13       13              
  Lines        1317     1320       +3     
==========================================
+ Hits         1136     1139       +3     
  Misses        181      181              
Impacted Files Coverage Δ
lib/client.js 90.93% <100.00%> (+0.02%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@robertsLando robertsLando changed the title fix: add commits from master feat: option to disable writeCache and fix leak in subscriptions Jun 29, 2023
@robertsLando
Copy link
Member Author

@vishnureddy17 I dunno if there are other cleaner ways to do this, I found some old PR that I merged were pointing to master, I noticed this just now and with this commit I'm moving them to main (squash).

@vishnureddy17
Copy link
Member

vishnureddy17 commented Jun 29, 2023

I don't think we should squash this one, we should rebase and merge. Might have to temporarily change the repository setting to allow this

@robertsLando
Copy link
Member Author

problem is commit doesn't respect conventional commit standard, I need to check if I can rename them someway

@vishnureddy17
Copy link
Member

you could rename them with an interactive rebase, but it may not be worth the effort.

If the choice is between squashing the commits or having commits that don't follow conventional commit form, I think the second one is better

@robertsLando
Copy link
Member Author

@vishnureddy17 I enabled rebase but GH tells me there are conflicts (even if this pr is even with main branch)

@vishnureddy17
Copy link
Member

In that case, could we squash merge but make sure that the titles of the underlying commits (including the PR numbers) are in the commit description? That way someone can still go through the commit history and find the relevant PRs in the future.

Once we are sure that everything is in main and master doesn't have anything important, I think we should delete master to avoid future confusion.

This is probably really annoying, I really appreciate your handling this :)

@robertsLando
Copy link
Member Author

Sure! No problem, I understand that :) I will do it on Monday

@robertsLando robertsLando merged commit c8aa654 into main Jul 3, 2023
4 of 5 checks passed
@robertsLando robertsLando deleted the fix-master branch July 3, 2023 09:07
@vishnureddy17
Copy link
Member

@robertsLando FYI, I just changed the branch protection rules to lock master so we avoid the confusion going forward.

I am not sure if it is safe to delete, but if it is, I think we should delete it.

@robertsLando
Copy link
Member Author

@vishnureddy17 agree about the lock, not sure about deleting it. I would keep it for now, anyway PR merge should fail right now. Not sure if there is an easier way to move all open PR from master to main, maybe someone ha built a tool for that?

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.

None yet

5 participants