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

refactor: merge bolt and guts_cli #394

Closed
wants to merge 2 commits into from
Closed

refactor: merge bolt and guts_cli #394

wants to merge 2 commits into from

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Jan 28, 2023

Points:

  1. There are lots of duplicated definitions between bolt and guts_cli, which is definitely not good.
  2. The implementation in guts_cli also has issue, please refer to fatal error: checkptr: converted pointer straddles multiple allocations #391. This refactoring can fix the issue.

Moved all common definitions into package internal/common. Majority other changes are basically mechanical changes.

Signed-off-by: Benjamin Wang wachao@vmware.com

@ahrtr ahrtr marked this pull request as draft January 28, 2023 06:41
@ahrtr ahrtr marked this pull request as ready for review January 28, 2023 07:26
@ptabor
Copy link
Contributor

ptabor commented Jan 28, 2023

I support the direction. Thank you for this refactoring.

The plan I was thinking about assumed:

  • releasing v1.3.6.
  • doing bigger refactoring as part of v1.4.0 line (such that potential regressions within 1.3.x line are easy to follow).

v1.3.5 is already 2.5 year old.

@ahrtr
Copy link
Member Author

ahrtr commented Jan 28, 2023

v1.3.5 is already 2.5 year old.

Actually v1.3.6 was released Jun 2, 2021.
https://github.com/etcd-io/bbolt/releases/tag/v1.3.6

I agree to release 1.3.7 firstly. Let me trigger a pipeline to verify bolt in etcd's pipeline, if there is no any issue, then we can tag v1.3.7

@ptabor
Copy link
Contributor

ptabor commented Jan 29, 2023

Indeed it was tagged (and used in etcd) but not publised on: https://github.com/etcd-io/bbolt/releases

@ahrtr ahrtr added this to the v1.4.0 milestone Jan 30, 2023
@ahrtr
Copy link
Member Author

ahrtr commented Jan 30, 2023

v1.3.7 is already released, this PR will only target v1.4.0.

@ahrtr ahrtr requested a review from ptabor January 30, 2023 21:42
@ahrtr
Copy link
Member Author

ahrtr commented Feb 14, 2023

@ptabor could you please take a look this PR? I am planning to fix the following two issues on top of this PR,

Points:
1. There are lots of duplicated definitions between bolt and
   guts_cli, which is definitely not good.
2. The implementation in guts_cli also has issue, please
   refer to #391.
   This refactoring can fix the issue.

Signed-off-by: Benjamin Wang <wachao@vmware.com>
Signed-off-by: Benjamin Wang <wachao@vmware.com>
@ptabor
Copy link
Contributor

ptabor commented Mar 2, 2023

@ahrtr : I assume its obsoleted by: #407.

@ahrtr
Copy link
Member Author

ahrtr commented Mar 2, 2023

@ahrtr : I assume its obsoleted by: #407.

@ptabor exactly.

@ahrtr
Copy link
Member Author

ahrtr commented Mar 2, 2023

Closing this PR, and let me continue to work on #407

@ahrtr ahrtr closed this Mar 2, 2023
@ahrtr ahrtr removed this from the v1.4.0 milestone Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants