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

How to check which series can be read/ written #648

Closed
wants to merge 8 commits into from
Closed

How to check which series can be read/ written #648

wants to merge 8 commits into from

Conversation

nicolai86
Copy link
Contributor

When creating new database users the cluster admin can set readFrom/ writeTo attributes.

However these attributes are not returned when executing
GET /db/anydb/users.

This makes maintaining granular series permissions a problem because I don't know which permissions a user currently has.

Now I'm wondering: was this by design, or did you just leave this out for a later point in time? Would you accept PR's which add these attributes to UserDetails ?

@jvshahid
Copy link
Contributor

Hi @nicolai86, I believe this was left out when permissions where added. We love prs. Let us know if you need help with it.

@nicolai86
Copy link
Contributor Author

@jvshahid I've finished a first draft, tests seem to pass & manual testing indicates that the attributes are returned correctly.

However I'd like some feedback regarding coding style & general structure, as I might've been going down the wrong path while implenting the changes...

@jvshahid
Copy link
Contributor

@nicolai86 can you sign the cla at http://influxdb.com/community/cla.html so we can merge the pr. I'll take a look at the pr now

@jvshahid
Copy link
Contributor

lgtm. Ideally I'd like to see a test to go with the pr. A test would probably require changing the go client library which we use in the tests. If you feel like making that change as well, it'd be awesome. The assertion should probably be in this test

@jvshahid
Copy link
Contributor

Also, the test suite failed. Can you run make test and fix the errors

@nicolai86
Copy link
Contributor Author

Looks like I missed some instances of the changed struct initialisation. Didn't see that >.<
I'll see to fix those. thanks.

@nicolai86
Copy link
Contributor Author

@jvshahid I've added permission verification in 9c48cc9. Plus I hope the tests are green now.

Aside: I noticed that the tests seem to toggle locally, especially FAIL: leveldb_shard_datastore_test.go:15: LevelDbShardDatastoreSuite.SetUpSuite
Is that a problem you noticed too, or is it my setup?

@nicolai86
Copy link
Contributor Author

odd, looks like make test is not what travis executes. lets see...

@jvshahid
Copy link
Contributor

yeah, travis run the integration tests as well, I asked you to run make test because the unit tests were failing and is much faster to run

@nicolai86
Copy link
Contributor Author

integration tests really are quite slow...

@nicolai86
Copy link
Contributor Author

and off I go again, fixing the slow tests :)

@jvshahid
Copy link
Contributor

@nicolai86 it looks like the only test that's currently failing is SingleServerSuite.TestUserWritePermissions you can run the test by doing make integration_test only=SingleServerSuite.TestUserWritePermissions

@jvshahid
Copy link
Contributor

you can add verbose=on to see the output

@nicolai86
Copy link
Contributor Author

Ah thanks for the info I was looking for that. will fix it in a second

@nicolai86
Copy link
Contributor Author

Didn't know how to run a single integration test

@nicolai86
Copy link
Contributor Author

@jvshahid thanks for the heads up, I'm new to golang tests /w tags (e.g. integration) and the codebase is bigger than most of my golang projects!

@nicolai86
Copy link
Contributor Author

yay tests are green. Now back any other feedback you might have?

@jvshahid
Copy link
Contributor

jvshahid commented Jul 1, 2014

Closed by e5de7d3

@jvshahid jvshahid modified the milestones: 0.8.0, 0.7.4 Jul 24, 2014
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.

None yet

2 participants