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

pin update fixes #5265

Merged
merged 3 commits into from
Jul 30, 2018
Merged

pin update fixes #5265

merged 3 commits into from
Jul 30, 2018

Conversation

Stebalien
Copy link
Member

  • flush pins
  • correctly print output
  • add a sharness test

@Stebalien Stebalien requested a review from Kubuxu as a code owner July 20, 2018 01:16
@ghost ghost assigned Stebalien Jul 20, 2018
@ghost ghost added the status/in-progress In progress label Jul 20, 2018
@Stebalien Stebalien requested a review from schomatis July 20, 2018 01:17
@Stebalien Stebalien added need/review Needs a review and removed status/in-progress In progress labels Jul 20, 2018
@schomatis
Copy link
Contributor

@Stebalien What issue/PR/doc would you recommend to learn about the pinning system architecture to better understand the Flush call?

@Stebalien
Copy link
Member Author

Don't know. Looking at the code, it looks like pins are cached in-memory until the user calls Flush. At that point, they're packed into a single protonode and stored in the datastore. corerepo.Pin and corerepo.Unpin automatically flush (and have a bunch of other interesting logic) but pinner.{Pin,Unpin,Update} don't.

@schomatis
Copy link
Contributor

There's some interesting logic down there..

@schomatis
Copy link
Contributor

corerepo.Pin and corerepo.Unpin automatically flush (and have a bunch of other interesting logic) but pinner.{Pin,Unpin,Update} don't.

Sounds reasonable.

I can confirm this fixes some weird unwrapOutput error.

LGTM, minus the rebase, probably because I took to long to review this, sorry.

@schomatis schomatis added needs rebase and removed need/review Needs a review labels Jul 25, 2018
@ghost ghost added the status/in-progress In progress label Jul 25, 2018
@whyrusleeping
Copy link
Member

will merge in 0.4.18

@whyrusleeping
Copy link
Member

@Stebalien needs a rebase, sorry.

@Stebalien Stebalien removed the status/in-progress In progress label Jul 29, 2018
@Stebalien @whyrusleeping
correctly handle the output of pin update
82e006e 
fixes #4999

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@Stebalien @whyrusleeping
flush pins in the pin update command
5ca0302 
fixes #5264

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@Stebalien @whyrusleeping
test pin update
e51588e 
tests #4999 and #5264

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@ghost ghost assigned whyrusleeping Jul 30, 2018
@ghost ghost added the status/in-progress In progress label Jul 30, 2018
@whyrusleeping whyrusleeping merged commit c16a38e into master Jul 30, 2018
@ghost ghost removed the status/in-progress In progress label Jul 30, 2018
@whyrusleeping whyrusleeping deleted the fix/4999 branch July 30, 2018 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schomatis schomatis schomatis approved these changes

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees

@Stebalien Stebalien

@whyrusleeping whyrusleeping

Labels
RFM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Stebalien @schomatis @whyrusleeping