RCM API: use the new PushComposeRequest method #231
Conversation
msehnout
force-pushed
the
rcm-api-contd-2
branch
3 times, most recently
from
7e9269f
to
87c0c61
Compare
msehnout
force-pushed
the
rcm-api-contd-2
branch
from
87c0c61
to
3e5914f
Compare
msehnout
force-pushed
the
rcm-api-contd-2
branch
from
3e5914f
to
7257081
Compare
msehnout
force-pushed
the
rcm-api-contd-2
branch
from
dfe8881
to
282dfd2
Compare
msehnout
force-pushed
the
rcm-api-contd-2
branch
2 times, most recently
from
5a31e6a
to
e24537a
Compare
| func main() { | ||
| // Tests that the package wrapping dnf-json works as expected | ||
| dir, err := setUpTemporaryRepository() | ||
| defer func(dir string) { |
There was a problem hiding this comment.
This just a small nit - I don't think the dir parameter is needed, I'm fine with the function being closure.
There was a problem hiding this comment.
That's (probably) correct, happy to take a clean-up in a follow-up PR.
| } | ||
| id, exists := mapping[*dist] | ||
| if !exists { | ||
| return "", &CustomTypeError{reason: "Distribution does not have a module platform ID assigned"} |
There was a problem hiding this comment.
I think we should just panic here, this is clearly our mistake.
There was a problem hiding this comment.
Thinking about it again, I think we should keep it like this because we have the same logic in other transformation functions in this package.
There was a problem hiding this comment.
I would have preferred to look up the distro and query that, but let's not block on that. Not a fan of hardcoding strings more than once.
| @@ -72,7 +72,7 @@ func main() { | |||
| // Optionally run RCM API as well as Weldr API | |||
| if len(listeners) == 3 { | |||
| rcmListener := listeners[2] | |||
| rcmAPI := rcm.New(logger, store) | |||
| rcmAPI := rcm.New(logger, store, rpmmd.NewRPMMD()) | |||
There was a problem hiding this comment.
rpm variable can be reused here instead of creating new RPMMD instance
There was a problem hiding this comment.
yeah, I was thinking about it. Is it safe to share it?
There was a problem hiding this comment.
type rpmmdImpl struct{} - it doesn't have much state. :-D
this is needed for RCM API because the user will provide only the URL and we need to fetch the checksum ourselves
this test will create a temporary directory, create repo inside, then fetch the checksum, and finally clean up the directory
There was a bug in the previous implementation which used to pass the argument as a value but that does not work because we need to change the value of it. The new implementation uses pass by reference. Create a test to cover this scenario.
it used a wrong mapping, replace it with the right one
The change also requires customizations in the error handling, as some errors are now handled automatically by the custom unmarshaler. Include a note about HTTP return types.
the new version is better suited for the need of this API
previously these were provided, but in case of RCM API they are not, therefore we fetch them automatically
the name was misleading because the function could do more than just download package list. In PushComposeRequest it is also used to fetch checksums for the repositories, therefore I decided to rename it to reflect this usage.
When I run make build I expect to build all the code we have to make sure it still compiles just fine.
This is needed for unit tests, because it wasn't possible to mock the rpmmd module before. This also requires that the checksum is moved to the compose request and evaluated in the endpoint handler instead of push compose. I think it makes sense to have the checksum in the compose request directly. Also a "module platform ID" is required now, but we don't have the "global" distribution any more, so this patch introduces mapping from a distribution to the module platform ID.
msehnout
force-pushed
the
rcm-api-contd-2
branch
from
e24537a
to
10e87ae
Compare
There was a problem hiding this comment.
LGTM, thanks! When @teg approves it's imho ready to be merged.
| func main() { | ||
| // Tests that the package wrapping dnf-json works as expected | ||
| dir, err := setUpTemporaryRepository() | ||
| defer func(dir string) { |
There was a problem hiding this comment.
That's (probably) correct, happy to take a clean-up in a follow-up PR.
| } | ||
| id, exists := mapping[*dist] | ||
| if !exists { | ||
| return "", &CustomTypeError{reason: "Distribution does not have a module platform ID assigned"} |
There was a problem hiding this comment.
I would have preferred to look up the distro and query that, but let's not block on that. Not a fan of hardcoding strings more than once.
see individual commits