Skip to content
This repository has been archived by the owner on Sep 12, 2018. It is now read-only.

Use existing arch/os information for searches and index operations #853

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

estesp
Copy link
Contributor

@estesp estesp commented Dec 15, 2014

This code is a first step in making the current registry aware of
images for non-Intel architecture. While os==linux today, if
we are adding architecture we might as well add both arch and
os for future use by Microsoft and/or others where os != linux.

Signed-off-by: Pradipta Kr. Banerjee [email protected]
Signed-off-by: Phil Estes [email protected]

@wking
Copy link
Contributor

wking commented Dec 15, 2014

On Mon, Dec 15, 2014 at 10:44:57AM -0800, Phil Estes wrote:

This code is a first step in making the current registry aware of
images for non-Intel architecture. While os==linux today, if we are
adding architecture we might as well add both arch and os for
future use by Microsoft and/or others where os != linux.

Sounds good to me, but I think getting the new model documented 1
should happen before the implementation here. The public hub uses a
different search implementation, and we don't want the two
implementations growing too far out of sync.

@estesp
Copy link
Contributor Author

estesp commented Dec 15, 2014

@wking good point--although the API interactions do not change at all (hmm--I did change the search result JSON to add arch/os to the response), I suppose it makes sense to document the assumption (e.g. arch/os == amd64/linux) unless a user_agent string containing arch/ and os/ exist in the request. Also, I can augment the example JSON to show that Architecture and OS are useful values with this change, and are used today in the docker daemon (set by the graph to GOOS and GOARCH: https://github.com/docker/docker/blob/master/graph/graph.go#L125-L126).

I will put together a doc PR for the search response and other notes about over in docker, as it looks like the docs are part of their github repo.

@estesp estesp force-pushed the add-arch-os-to-index-search branch 3 times, most recently from a0bdc2c to 36764aa Compare January 5, 2015 15:00
@estesp
Copy link
Contributor Author

estesp commented Jan 5, 2015

@wking Thought I would get to it before my holiday time off, but didn't--however, I've added a docs PR for the search API change. I also fixed all flake8 errors from the CI run that didn't show locally when I ran it, but still have CI failures that seem to be unrelated to my code..any input, suggestions welcome!

@dmp42
Copy link
Contributor

dmp42 commented Jan 5, 2015

You can ignore circleci, but the travis build report a number of errors that should be fixed, for example:
https://travis-ci.org/docker/docker-registry/jobs/45954757#L1205

alternatively, you should run the tests suite locally - python setup.py tests or tox (or TOX_INCLUDE='' TOX_LIBRARY='' tox)

@estesp estesp force-pushed the add-arch-os-to-index-search branch 3 times, most recently from 63d6099 to b875ea6 Compare January 5, 2015 22:38
This code is a first step in making the current registry aware of
images which are non-Intel architecture.  While os==linux today, if
we are adding architecture we might as well add the pair of arch and
os for future use by Microsoft where os=windows but arch=amd64.

Signed-off-by: Pradipta Kr. Banerjee <[email protected]>
Signed-off-by: Phil Estes <[email protected]>
@estesp estesp force-pushed the add-arch-os-to-index-search branch from b875ea6 to 0687003 Compare January 6, 2015 01:03
@estesp
Copy link
Contributor Author

estesp commented Jan 6, 2015

Thanks @dmp42. I've fixed the tests that were failing, including a potentially important catch for an issue created by the new code--the mocked up images used in a few tests [no idea if it would be hit in real usage, but worth handling properly] had no image data, so attempting to parse that on GET and PUT repository operations caused a FileNotFoundException that was causing an inadvertent code path that was wrong for both cases. I've fixed that in the code and now all tests are passing.

@dmp42
Copy link
Contributor

dmp42 commented Jan 6, 2015

@shin- @wking what do you think?

@wking
Copy link
Contributor

wking commented Jan 6, 2015

On Mon, Jan 05, 2015 at 06:22:14PM -0800, Olivier Gambier wrote:

@shin- @wking what do you think?

I still want the search result structure in the spec 1. Having
multiple quasi-official search implementations with different result
structures is just going to be confusing. Sharing the same endpoint
is not enough for client compatibility ;).

@estesp
Copy link
Contributor Author

estesp commented Jan 6, 2015

@wking agreed--might have gotten lost in the noise of earlier comments, but I opened moby/moby#9904 with the change to the search API spec. Of course I've only succeeded in confusing @jfrazelle so far given these changes aren't available in any registry yet :)

@jessfraz
Copy link

jessfraz commented Jan 6, 2015

lol sorry I am caught up now

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

Successfully merging this pull request may close these issues.

4 participants