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

Redo gconf #503

Merged
merged 23 commits into from
Apr 18, 2019
Merged

Redo gconf #503

merged 23 commits into from
Apr 18, 2019

Conversation

husio
Copy link
Contributor

@husio husio commented Apr 8, 2019

Cleanup gconf package configuration.

Instead of allowing for any key-value pairs inserted into gconf store,
require each module to use a custom protobuf configuration object that
will be stored and retrieved with the help of gconf package.

resolve #456

@husio husio requested a review from alpe April 8, 2019 12:57
@husio husio changed the title work in progress gconf cleanup Apr 8, 2019
@husio
Copy link
Contributor Author

husio commented Apr 8, 2019

There is one test failing that I cannot figure out. I have spend a lot of time on fixing tests and I cannot figure this one out.

--- FAIL: TestApp (0.00s)
    require.go:1159: 
                Error Trace:    app_test.go:60
                Error:          Should be true
                Test:           TestApp
                Messages:       not found tag 636173683AB1EA5ED23D59CF270DCA939BA0CB1A921459D45A in cash:27D2C706A78ED16EF96126AC6362E4E59616215A;cash:67FA696F97F18BF37E9CFE0D354CED0DC9904078;cash:71468ACE7A8FF9EDE9BE6CFA568680113ECE0B53;sigs:67FA696F97F18BF37E9CFE0D354CED0DC9904078                                  
FAIL
FAIL    github.com/iov-one/weave/cmd/bnsd/app   0.045s

@husio husio mentioned this pull request Apr 8, 2019
6 tasks
@husio husio marked this pull request as ready for review April 8, 2019 13:44
@alpe alpe self-assigned this Apr 9, 2019
@husio husio closed this Apr 9, 2019
@husio
Copy link
Contributor Author

husio commented Apr 9, 2019

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Nice start. This is actually my preferred approach.

I think a bit more polish and I would give it a 👍
Will add comments/examples about polish

cmd/bnsd/app/app_performance_test.go Outdated Show resolved Hide resolved
x/cash/codec.proto Show resolved Hide resolved
x/cash/configuration.go Show resolved Hide resolved
x/cash/init.go Outdated Show resolved Hide resolved
gconf/gconf.go Outdated Show resolved Hide resolved
@ethanfrey
Copy link
Contributor

I'm reopening this one, as I used it as a basis for my new pr #534

Let's get this done

@codecov-io

This comment has been minimized.

@husio husio unassigned alpe Apr 18, 2019
husio added 3 commits April 18, 2019 13:25
- enhance error messages
- move message code to `msg.go`
- move handler code to `handlers.go`
- register configuration handler together with other message handlers
@husio husio force-pushed the cleanup_gconf_2_issue_456 branch from aff60d3 to 4fe62e9 Compare April 18, 2019 11:26
@husio husio changed the title gconf cleanup Redo gconf Apr 18, 2019
@husio
Copy link
Contributor Author

husio commented Apr 18, 2019

I think this is ready. @alpe @ethanfrey please review it again. 🤞

@ethanfrey
Copy link
Contributor

Will review now

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good, but please fix broken benchmarks before merging

CHANGELOG.md Show resolved Hide resolved
cmd/bcpd/app/app_test.go Outdated Show resolved Hide resolved
cmd/bnsd/app/app_performance_test.go Outdated Show resolved Hide resolved
cmd/bnsd/app/app_test.go Outdated Show resolved Hide resolved
gconf/gconf_test.go Show resolved Hide resolved
// message must be signed by an owner in order to be authorized to apply the
// change.
type OwnedConfig interface {
Unmarshaler
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 referring to Configuration here is cleaner. It shows we extend the Configuration interface with one more method

gconf/handler.go Show resolved Hide resolved
gconf/handler.go Show resolved Hide resolved
gconf/handler_test.go Show resolved Hide resolved
x/cash/msg.go Show resolved Hide resolved
@ethanfrey ethanfrey merged commit 9e06fb1 into master Apr 18, 2019
@ethanfrey ethanfrey deleted the cleanup_gconf_2_issue_456 branch April 18, 2019 15:02
@husio
Copy link
Contributor Author

husio commented Apr 19, 2019

🎉 :shipit: 🎉

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

Successfully merging this pull request may close these issues.

Clean up gconf implementation
4 participants