Skip to content
This repository has been archived by the owner on Apr 26, 2021. It is now read-only.

Added support to update repository data #168

Merged
merged 1 commit into from
Sep 19, 2014

Conversation

ricardodani
Copy link
Contributor

Its important to notice that Update does not renames the repository, cause the Rename method removes the old repository in mongo, inserts the new and renames the filesystem repository. While Update just do a simple database update. So its important to maintain this two methods separeted to avoid an intermitent state on database in cases that one of them doesn`t works.

@andrewsmedina
Copy link
Contributor

@ricardodani I think that you can remove the rename handler and merge it into the update handler. So you can use the /repository/:name path instead /reository/:name/update.

@ricardodani
Copy link
Contributor Author

@andrewsmedina I preferred to do it this way because the repository.Rename doesn't updates the repository, in fact, this method removes and add`s a new one in the database and moves the git repository in filesystem.

Thinking thus, Rename method acts very different from the Update which is what really makes an Update operation in the database. My concern is not with the dogma of a method to do just one operation. My concern is with the consistency between the operations it processes and the order of things.

For example:

  • We attempt a rename operation: it is executed successfully, then the update is done and fails, what to do? Returns the repository in a flashing or tries to "rollback" the rename?
  • Other alternative: We first started doing the update. It runs successfully, the rename fails. Do we do the "rollback" of the update? Or also return the repository "half edited"?

Thinking of giving the things simple, I separated the method in these two.

What do you think about this? Would you still recommend using only one route?

Thanks

@andrewsmedina
Copy link
Contributor

@ricardodani first of all the PUT in /repository/:name call the rename handler because when it was implemented the only attribute that the repository had was the name. And currently the repository.Rename is broken, this method will erase all other attributes on rename.

My idea is merge update and rename, and only do the "filesystem" stuff when the name will be changed.

A way to fix the "rollback" is to make all handler actions as atomic actions for this handler. tsuru has a package to do it. And it is a current problem in the rename handler.

I think that this way will make the api more consistent.

@fsouza
Copy link
Contributor

fsouza commented Sep 18, 2014

If we want to rename a repository, we should stop using the name as the id of the document. That remove + insert is ugly.

@scorphus
Copy link
Contributor

It lacks a partial update test, but I believe it works. Apart from the ugliness of the insert+remove, it LGTM.

}
return fs.Filesystem().Rename(barePath(oldName), barePath(newName))
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.

I think err should be returned instead, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you notice the erros when they happen, they are aways returned imediatly, in this case i have shure that no errors ocurred, so, return no error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand what you said now. Fixed. But in a different way, returning error explicity on the condition, not on the bottom of the method (which is explict that no errors ocurred)

Copy link
Contributor

Choose a reason for hiding this comment

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

It won't build with no return at the end, mate 😉

andrewsmedina added a commit that referenced this pull request Sep 19, 2014
Added support to update repository data
@andrewsmedina andrewsmedina merged commit 5ea3142 into tsuru:master Sep 19, 2014
@andrewsmedina
Copy link
Contributor

@ricardodani thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants