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: Wire meta.DB through codebase #94

Conversation

twavv
Copy link
Contributor

@twavv twavv commented Apr 27, 2023

This is obviously a pretty big change. Want to test it decently thoroughly and possibly add some more e2e type tests.

Next PR in stack will implement the import existing metadata from refs functionality.

@aviator-app
Copy link
Contributor

aviator-app bot commented Apr 27, 2023

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.

Stack

  1. 👉 refactor: Wire meta.DB through codebase #94 👈 (this pr)

@twavv twavv requested a review from draftcode April 27, 2023 14:56
@twavv twavv marked this pull request as ready for review April 27, 2023 14:56
@twavv twavv changed the title [MER-2210] Wire meta.DB through codebase refactor: Wire meta.DB through codebase Apr 27, 2023
}
return base, nil
}
//// BaseCommit determines the base commit for the given branch.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the intention for this comment out?

func branchMetaRefName(branchName string) string {
return branchMetaRefPrefix + branchName
}
//func FindStackRoot(branches map[string]Branch, name string) (Branch, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the intention for the comment out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just forgot to delete the function after deleting its usages. My bad!

if err != nil {
return err
}
tx := db.WriteTx()
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be ReadTx.

if err != nil {
return err
}
tx := db.WriteTx()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be committed.

if err := meta.WriteBranch(repo, *br); err != nil {
return err
}
tx.SetBranch(*br)
}
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, you want to cancel cu here. But if we change the Abort to be called even after Commit (like I commented in the other PR), we can omit the cancel.

@@ -316,6 +323,9 @@ base branch.
if err := writeStackSyncState(repo, nil); err != nil {
return errors.Wrap(err, "failed to write stack sync state")
}
if err := tx.Commit(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, we need to cancel cu every time we call Commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah just cancelled this to always defer tx.Abort().

Parent: meta.BranchState{
Name: parentBranchName,
Trunk: isBranchFromTrunk,
Head: parentHead,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix!

cmd/av/pr.go Outdated
Comment on lines 63 to 65
tx := db.WriteTx()
var cu cleanup.Cleanup
defer cu.Cleanup()
Copy link
Contributor

Choose a reason for hiding this comment

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

Commit and cancel.

@aviator-app
Copy link
Contributor

aviator-app bot commented May 3, 2023

This pull request failed to merge: top queued PR in the stack failed to merge. Remove the blocked label to re-queue.

@aviator-app aviator-app bot added the blocked label May 3, 2023
@twavv twavv force-pushed the travis/mer-2205-create-av-cli-db-interface branch from 5718042 to 68f461b Compare May 3, 2023 17:36
@twavv twavv force-pushed the travis/mer-2210-wire-db-store branch from 25938f5 to 253fb67 Compare May 3, 2023 17:42
@twavv twavv removed the blocked label May 3, 2023
@aviator-app aviator-app bot closed this May 3, 2023
@twavv twavv deleted the travis/mer-2210-wire-db-store branch November 8, 2023 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants