-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: adding POST to candlepin #14
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The container will reject POST in insecure mode
I cannot reproduce this; I tried a slightly modified version of your example, and it works fine:
def test_candlepin_env(candlepin):
env_uno = {"id": "env-id-1", "name": "env-name-1", "description": "Testing environment num. 1"}
foo = candlepin.post("owners/donaldduck/environments", env_uno, auth=("admin", "admin"))
bar = candlepin.get(f"environments/{env_uno['id']}")
assert bar.json() == foo.json()
Would you please provide a log when running the test using pytest --log-level=DEBUG
?
The current version also sets "admin"/"admin" as credentials for every call made to Candlepin when using the container; I don't think this is a good idea, as tests may be using non-admin users to communicate with Candlepin. As shown above, you can pass already now parameters to the requests done with requests
, and IMHO doing it them when needed seems a better idea.
The addition of post()
to RestClient
and Candlepin
seems fine to me; would you please keep it in its own commit, with a nicer commit e.g.
feat: add POST support to RestClient and Candlepin
Add the possibility to perform POST calls to Candlepin,
wiring the support to RestClient.
Signed-off-by: ...
(and don't forget the sign-off, right now it is not checked)
Thanks!
Wow thanks for the quick review. Yeah I completely forgot that you can add the auth outside of the Session object - that simplifies this a lot. Turns out that the insecure mode bug was due to me messing with my laptop too much. We can ignore that. Everything else has been updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The post()
additions LGTM, thanks.
The other changes (i.e. verify the connections to the embedded Candlepin) seem generally OK, although there are few things I'd like to improve before we get to those. Would you please move them to their own PR, so this one for post()
can be merged?
Thanks!
Add the possibility to perform POST calls to Candlepin, wiring the support to RestClient. Signed-off-by: J.C. Molet <[email protected]> splitting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Here I added a POST command so that we could test RedHatInsights/rhc#121
The container will reject POST in insecure mode so additional code was needed to perform REST calls validated against the container's cert. I will assume that pre-existing candlepins will be already added to the system's ca-trust.
This is testable with something like:
I added this as a unit test using pytester, and it passes, but I'm not sure how pytester works - it blows up after the test? Let me know if I should include that in this PR.