According to the cases in tests, seems query is a complex data structure for an API to use, can we make some improvements on this? Or at least document it?

LightBlue documentation should be referenced to know how to write query. Document could be out-dated if the documentation gets updated in upstream.

This is a lower level API to query LightBlue. When using this API to implement docker image rebuild with LightBlue, there will be concrete query dict written and passed via this query parameter, and that could also be an example how the query looks like in practice.

I think I can add a short description of query and give the link of upstream documentation. That could be helpful hopefully. What do you think?

Any reason to default to False really?

I would suggest you add variables for schema versions to be used in these queries for containerRepository and containerImage. You use default schema version which means it's not pinned and could change for you.

This will throw KeyError exception if there is any error message from LB - for example if the query is wrong.

I would suggest making this and find_container_repositories into private methods and provide a public method with specific parameters for specific use cases. I.e something like

get_repositories_with_content_set(self, content_set)
get_images_in_repository(self, repository_id)
etc

Is the query supposed to be full query including the projection, sort etc? If so - that's confusing use since you can see query is just one part of the request:
https://docs.lightblue.io/cookbook/find_data.html

I'm OK with cqi's explanation here. +1

+1

According to cqi's comment on another part of PR, I think this is what he will do in the end. So far the task is to write a class to query lightblue. I think we can easily change the API here once we will find out high-level use-cases.

Fair enough, but I'd still suggest we start with private methods (i.e. __find_container_images etc)

I'd recommend adding fail-tests (i.e. tests simulating request.post failure and errors from LB)

This option is for the test configuration. I thought in test environment to run freshmaker, disabling SSL verify could make things simple. I'm happy to change it to True as default, if we need to force to verify SSL in all environments.

I'm OK with having the dev and test configuration to not verify ssl. Just keep the BaseConfiguration set to True.

Yes. Query should be a full query in a POST request to LightBlue, and this class does not make GET request to LightBlue, instead, only POST.

Sorry, I don't get your point here, why do you think it might a confusing use?

Hi sochotni, as Jan mentioned above, we need to find out high-level use cases next. So far, it is not clear enough to know if this class is enough to include all code interacting with LightBlue, or need another place to include code to implement higher level features, for example, those methods you listed. As the first step, just keep this class as the very low level API, and make changes to it if necessary.

OK, so an example of the query parameter will be following:
{
"objectType": "containerRepository",
"query": {
"field": "_id",
"regex": "."
},
"projection": [
{"field": "", "include": true, "recursive": true}

]

}

In other words - only one part of the query parameter is actually the lightblue query. Rest of it is other things.

I realize - it was just a suggestion. Having a very generic method be part of your class API means that you can't control error handling very well. Using private method designator (__ prefix) doesn't prevent you from working on higher level features later. It just means "hey, don't use this method directly from outside" to the users of your class. It can still be used by subclasses easily.

Hi sochotni, cool, get your point. You are considering from the view of inheritance, whereas I was thinking of it from view of composition only :) Your idea makes sense to me.

Hi sochotni,

I see. The reason of why I use the full query is to allow the ability of selecting part of fields. Because not the whole image or repository data is required always, for example, maybe _id and brew are just required for starting rebuild of docker image.

To avoid confusion, how about make query, fields (the projection), range, and sort as separate parameters? Is it worth to do?

That could be one way to do things. Simpler approach: just rename the parameter to "request"? :-)

We really should create a separate python library for interacting with Lightblue that we could all use...

Is it ready for another review?

Is it ready for another review?

@jkaluza yes

We really should create a separate python library for interacting with Lightblue that we could all use...

@sochotni I agree indeed. I searched before, so far, haven't found such a Python library. We could do it :)

Seems my previous comment describing the updated patch includes is lost :( I can't find it in this PR. Now, let me recover it

updates

• handle error returned from LightBlue
• add tests for the error handler
• rename parameters' name in method find_container_* and _make_request to avoid potential confusion, and add description for parameters
• specify version in URL to request data
• fix _make_request. Content-Type header is required in request

As long as you understand this has to be configurable. It will basically change every other month :-)

ping for review.

+1

+1 - I am sure there will be more changes later but let's get this in