Skip to content

Capabilities api#14

Merged
matejak merged 4 commits intoEnterpriseyIntranet:masterfrom
danil-topchiy:capabilities_api
Jan 17, 2019
Merged

Capabilities api#14
matejak merged 4 commits intoEnterpriseyIntranet:masterfrom
danil-topchiy:capabilities_api

Conversation

@danil-topchiy
Copy link

Add class with capabilities api endpoint
Based on #11, will rebase to master after #11 will be merged


def test_get_capabilities(self):
res = self.nxc.get_capabilities()
assert res['ocs']['meta']['statuscode'] == self.SUCCESS_CODE
Copy link

Choose a reason for hiding this comment

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

This test doesn't test much, do you have any ideas how to change that? I understand that capabilities may vary on the test environment, but if we add e.g. env vars to it, we may squeeze something out of it.

Copy link
Author

Choose a reason for hiding this comment

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

Agree, one of the main nextcloud related params, which can be set in docker-compose and cannot be changed is nextcloud version, so added it to python-api service environment variable and specified it in nextcloud Dockerfile. Amended the change to check it in test

Copy link

Choose a reason for hiding this comment

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

Great work, I propose one modification - don't check for the whole version, but go only for the major version (i.e. the X in v. X.Y.Z). Major versions don't change so often, they are part of the image name, and they test the capability query anyway. On recent-enough Docker setups, you can parametrize the FROM command using an argument that you can propagate to an environmental variable, so you don't have to repeat the literal nextcloud version on multiple places.

Copy link
Author

Choose a reason for hiding this comment

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

Done, moved nextcloud version to parameter and check only major version in test.

@danil-topchiy danil-topchiy force-pushed the capabilities_api branch 4 times, most recently from 4f70b2d to 23db46d Compare January 8, 2019 01:24
@matejak
Copy link

matejak commented Jan 14, 2019

Hello Danil, I generally like your PR, but it needs some ironing out. If you remove all containers, pull & rebuild, it is likely that tests won't pass due to nextcloud app not initialized properly. I suggest to throw away postgres in favor to sqlite as outlined in #12, it helped me and also solves the problem with the postgres persistence side-effect.
Next, you have spurious NEXTCLOUD_USERNAME=admin and NEXTCLOUD_PASSWORD=admin that aren't used elsewhere in the .env file,
Aside from that, the PR is good for merge!

@danil-topchiy
Copy link
Author

Hello Matej, thanks for review! I switched from postgres to sqlite as in #12 and deleted unused variables.
Also, after changing to sqlite in groupfolders app tests, nextcloud started to return strings instead of integers as before. Didn't investigate it much, just changed transforming both values to strings.

@matejak matejak mentioned this pull request Jan 15, 2019
@matejak
Copy link

matejak commented Jan 15, 2019

It is great that we discovered ourselves that those data types may be somehow arbitrary and that we have to pay attention to them.
Aside from this, the PR looks good. I will merge the LDAP one first, then please rebase this one, and I will merge it.

And one more thing, as we have touched the test setup, you can get rid of the alias declaration in the networks configuration, as aliases that have the same name as the service are redundant, amd python-api is the client, not the server, so nobody is interested in that container's hostname.

@danil-topchiy danil-topchiy force-pushed the capabilities_api branch 2 times, most recently from f2beae7 to 16aa9e3 Compare January 15, 2019 22:03
@danil-topchiy
Copy link
Author

I got rid of the aliases, thanks, and will rebase the branch after #17 merge.

@matejak
Copy link

matejak commented Jan 16, 2019

Great, I appreciate that. I see that you took the 8080 port mapping out too, I agree with that, but I also learned that it may come in handy when debugging e.g. installation issues. So if you later want to add it back, don't hesitate.

Danil added 4 commits January 17, 2019 00:35
@danil-topchiy
Copy link
Author

Yeah, I just left ports uncommitted on local, but it should probably be in repository too. Returned it and rebased.

@scrutinizer-notifier
Copy link

The inspection completed: 3 updated code elements

@matejak
Copy link

matejak commented Jan 17, 2019

Good job!

@matejak matejak merged commit ed5279d into EnterpriseyIntranet:master Jan 17, 2019
@danil-topchiy danil-topchiy deleted the capabilities_api branch February 3, 2019 16:47
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.

3 participants