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

PDC-645 Refactor compose/package #23

Merged
merged 1 commit into from
Aug 12, 2015

Conversation

ycheng-aa
Copy link
Contributor

Add 2 new end points to take old compose/package
endpoint function.
One is for a given release and srpm_name list all
composes that contain the package (and include its version).
Another is given a compose C and a package, find the
latest compose older than C which contains a
different version of the package.
Keep the old endpoint same for a while to don't break
existing scripts.

@lubomir
Copy link
Member

lubomir commented Aug 6, 2015

Couple notes:

  • generally this looks good
  • the APIs don't show up in API root, so they are hard to discover
  • I think we should drop the to_dict option by picking one behaviour and sticking with it

@ycheng-aa
Copy link
Contributor Author

@lubomir The "I think we should drop the to_dict option by picking one behaviour and sticking with it", you means drop to_dict only for these 2 new endpoints or for all endpoints including compose/package?

@lubomir
Copy link
Member

lubomir commented Aug 6, 2015

@ycheng-aa Just here, we should keep compose/package unmodified for backwards compatibility. Just simplify the new APIs. I don't think there are any business reason why someone should need this option, we just need a good default.

@ycheng-aa
Copy link
Contributor Author

@lubomir As discussed, I didn't add these 2 end points to API root page issue. And set to_dict to False first for these 2 in 9982836.

@lubomir
Copy link
Member

lubomir commented Aug 6, 2015

The documentation on FindLatestComposeByComposeRPM is a bit imprecise: the view always returns a single compose, so there is no need to describe what the ordering is, and the return value is always a single JSON object. The latest argument also does not make much sense there, and to_json is forgotten (line 1288).

@ycheng-aa
Copy link
Contributor Author

@lubomir Fixed in ada1f64. Please review.
My original understanding is the which one is 'latest' should depend on the order that described. First define the order, then pick the latest one according to the order. If not like this, remove the order part is good. Anyway, I removed it in this commit.

@lubomir
Copy link
Member

lubomir commented Aug 7, 2015

@ycheng-aa You are right, the order still determines which compose is latest. Maybe it needs to be documented, I'm not sure.

@ycheng-aa ycheng-aa force-pushed the PDC-645_refactor_compose_package branch from ada1f64 to e4d3005 Compare August 7, 2015 06:53
@ycheng-aa
Copy link
Contributor Author

@lubomir Ok, I reverted the documentation about the "order" for FindLatestComposeByComposeRPM.

@sochotnicky
Copy link

@lubomir FWIW the to_dict was requested earlier to simplify figuring out exact NVR bits without having to parse it

@ycheng-aa
Copy link
Contributor Author

@sochotnicky @lubomir So the final decision is keep "to_dict" or what?

@erichuanggit
Copy link
Collaborator

I would agree with stano. let's keep the original parameters since all these requirements were coming from clients.

@ycheng-aa ycheng-aa force-pushed the PDC-645_refactor_compose_package branch 3 times, most recently from cc4c118 to f169786 Compare August 11, 2015 11:01
@ycheng-aa
Copy link
Contributor Author

@lubomir @erichuanggit I add to_dict back for code/tests/documentation and rebased it. Please help me review it. Thanks.

@lubomir
Copy link
Member

lubomir commented Aug 11, 2015

Looks pretty good to me. I would prefer the commit title to not start with the PDC-645 and instead for it to be somewhere in the commit message. It is not very useful for people without access to Jira, so it does not need such prominent place.

@ycheng-aa
Copy link
Contributor Author

I will change the commit message in final rebase. The coverage failed seems because messaging. It should not be related to this pull request.

@erichuanggit
Copy link
Collaborator

Looks good to me.

@lubomir
Copy link
Member

lubomir commented Aug 12, 2015

👍

Add 2 new end points to take old compose/package
endpoint function.
One is  for a given release and srpm_name list all
composes that contain the package (and include its version).
Another is given a compose C and a package, find the
latest compose older than C which contains a
different version of the package.
Keep the old endpoint same for a while to don't break
existing scripts.

JIRA: PDC-645
@ycheng-aa ycheng-aa force-pushed the PDC-645_refactor_compose_package branch from 1364abf to ba5b41d Compare August 12, 2015 09:37
ycheng-aa added a commit that referenced this pull request Aug 12, 2015
…ose_package

PDC-645 Refactor compose/package
@ycheng-aa ycheng-aa merged commit 4d13eb9 into master Aug 12, 2015
@ycheng-aa ycheng-aa deleted the PDC-645_refactor_compose_package branch August 12, 2015 09:39
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.

4 participants