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

Fixed generating sync_key_template path #249

Merged
merged 1 commit into from
Jun 29, 2016
Merged

Fixed generating sync_key_template path #249

merged 1 commit into from
Jun 29, 2016

Conversation

wsikorska-codilime
Copy link
Contributor

resource.Get("body") sometimes doesn't have all necessary fields to put them into generated path, so the data should be fetched from db.

@nati
Copy link
Contributor

nati commented Jun 9, 2016

We should do this when we make body in the transaction, otherwise we can't ensure consistency of data.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 59.039% when pulling d0f73d7 on wsikorska-codilime:fix_sync_key_path into 45699b3 on cloudwan:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 58.904% when pulling 2ececed on wsikorska-codilime:fix_sync_key_path into 45699b3 on cloudwan:master.

@wsikorska-codilime wsikorska-codilime force-pushed the fix_sync_key_path branch 2 times, most recently from e7408a8 to e6c9f7b Compare June 10, 2016 13:15
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 59.131% when pulling e6c9f7b on wsikorska-codilime:fix_sync_key_path into 45699b3 on cloudwan:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 58.865% when pulling aa0d52f on wsikorska-codilime:fix_sync_key_path into 45699b3 on cloudwan:master.

@nati
Copy link
Contributor

nati commented Jun 10, 2016

Could you make your commit message descriptive?
I'm not sure what is fixed..

@wsikorska-codilime
Copy link
Contributor Author

During state update the path in etcd was chosen based on schema.url prefix instead of sync_key_template path. If path in etcd (when it's based on sync_key_template) doesn't have the same prefix as resource url in api, then gohan doesn't watch state updates for that resource. The same happens in the case of monitoring update.

@nati
Copy link
Contributor

nati commented Jun 13, 2016

I got it. Could you put the statement in the commit message?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 59.122% when pulling d942185 on wsikorska-codilime:fix_sync_key_path into 6abc86c on cloudwan:master.

vozhyk-
vozhyk- reviewed Jun 15, 2016
View reviewed changes
schema/schema.go
@@ -393,7 +393,8 @@ func (schema *Schema) GetResourceIdFromPath(schemaPath string) string {
}
}

func GetSchemaByPath(path string) *Schema {
//Get schema by resource path (from API)
Copy link
Contributor

@vozhyk- vozhyk- Jun 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The comment should start with the function name[1]. golint will likely complain.

[1] https://golang.org/doc/effective_go.html#commentary ("doc comments")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I fixed comments.

@marcin-ptaszynski
Copy link
Contributor

Commit message looks malformatted. Please check https://git-scm.com/book/ch5-2.html

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 59.122% when pulling ba68bc5 on wsikorska-codilime:fix_sync_key_path into 6abc86c on cloudwan:master.

@wsikorska-codilime wsikorska-codilime force-pushed the fix_sync_key_path branch 2 times, most recently from 7ae1254 to 59004fd Compare June 17, 2016 22:49
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 59.161% when pulling 59004fd on wsikorska-codilime:fix_sync_key_path into c74a3ca on cloudwan:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 59.086% when pulling 59004fd on wsikorska-codilime:fix_sync_key_path into c74a3ca on cloudwan:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 59.113% when pulling 59004fd on wsikorska-codilime:fix_sync_key_path into c74a3ca on cloudwan:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 59.113% when pulling 879a5b4 on wsikorska-codilime:fix_sync_key_path into c74a3ca on cloudwan:master.

@marcin-ptaszynski
Copy link
Contributor

Hi @nati , what is the status of this PR? Can we merge it?

@nati
Copy link
Contributor

nati commented Jun 28, 2016

@marcin-ptaszynski let me review again. Does this look good for you?

@nati
Copy link
Contributor

nati commented Jun 28, 2016

Commit messege need to be updated.
How about this?

During state update the path in etcd was chosen based on schema.url prefix instead of sync_key_template path. If path in etcd (when it's based on sync_key_template) doesn't have the same prefix as resource url in api, then gohan doesn't watch state updates for that resource. The same happens in the case of monitoring update.
In this commit, we apply sync_key_template for sync path.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 59.21% when pulling 168837c on wsikorska-codilime:fix_sync_key_path into 83c0b1e on cloudwan:master.

@wsikorska-codilime
Copy link
Contributor Author

I changed commit message and rebased to current version of master branch.

nati
nati reviewed Jun 29, 2016
View reviewed changes
schema/schema.go
schemaByPath = schema
break
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

} else if {

@wsikorska-codilime
Fixed generating sync_key_template path
65adff9 
Fixed looking for path in state and monitoring update

Fixed not getting resouce url from db

Added tests for schema urls

Fixes that path in etcd monitoring and state update should be
based on sync_key_template

Fixed some lint errors

Fix in test

Refactoring in GetSchema functions

During state update the path in etcd was chosen based on schema.url
prefix instead of sync_key_template path. If path in etcd (when it's
based on sync_key_template) doesn't have the same prefix as resource
url in api, then gohan doesn't watch state updates for that resource.
The same happens in the case of monitoring update.
In this commit, we apply sync_key_template for sync path.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 59.199% when pulling 65adff9 on wsikorska-codilime:fix_sync_key_path into 83c0b1e on cloudwan:master.

@nati
Copy link
Contributor

nati commented Jun 29, 2016

ok thanks!

@nati nati merged commit 8ddd747 into cloudwan:master Jun 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wsikorska-codilime @nati @coveralls @marcin-ptaszynski @vozhyk-