-
Notifications
You must be signed in to change notification settings - Fork 117
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
Add possibility to specify download-utility #16912
Conversation
Can one of the admins verify this patch? |
if download_utility is not None: | ||
options['download-utility'] = download_utility |
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.
@dosas Do you intent to add some test to cover this option?
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.
Obviously I did not intent to ;) but if you point me in the right direction I can add a test.
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.
Sure, this tests can be added to any of tests/foreman/{api,cli,ui}/test_registration.py
, and you can check of any e2e marked tests present in this modules for reference.
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.
@Gauravtalreja1 I thought you were talking about a test in tests/robottelo
...
I doubt that we can add a meaningful e2e test for this on rhel operating systems because this option was specifically added for debian based operating systems where instead of curl wget is installed on default.
Or are you talking about a test where we just set the option explicitly to curl?
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.
@dosas yes, I get that, but I meant like we've option to install wget first on rhel/centos as part of test setup, then running a registration command with download_utility=wget
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.
@Gauravtalreja1 I have added a test but I have no idea how (and where in the setup) I can install a package on an unregistered contenthost? Could you give me a hint?
Also which ID should I use for the new test?
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.
@dosas For the test ID: simply generate a new UUID. See https://github.com/SatelliteQE/robottelo/wiki/Robottelo-Contributing-Guidelines#required-docstring-content
To ensure the content is already on the host:
- ensure that the host already has required tooling installed or
- Write a fixture, register the host, enable repositories if needed, install tooling, unregister the host.
Our hosts already have both tools installed, however I do not like the fact that they come with pre-installed, non-default packages. In my view, robottelo should assume a host with a minimal installation. By minimal installation, I mean either the Minimal Install
or Core
package groups (dnf group info "Core"). I would prefer going with 2.
, however it is not ideal to register twice per test.
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.
@ogajduse I fixed the uuid.
If we go with option 1. it should already be possible to run the PRT on this test?
tests/foreman/cli/test_registration.py::test_positive_register_download_utility
would you mind triggering it?
in host registration see also theforeman/foreman#9808
978b1ce
to
a7a7f4c
Compare
PRT Result
|
PRT Result
|
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.
Thank you, @dosas. Let's get it in.
* Add possibility to specify download-utility in host registration see also theforeman/foreman#9808 * Add test for registration property download utility (cherry picked from commit 8805451)
* Add possibility to specify download-utility in host registration see also theforeman/foreman#9808 * Add test for registration property download utility (cherry picked from commit 8805451)
Add possibility to specify download-utility (#16912) * Add possibility to specify download-utility in host registration see also theforeman/foreman#9808 * Add test for registration property download utility (cherry picked from commit 8805451) Co-authored-by: dosas <[email protected]>
Problem Statement
newly added download utility option missing theforeman/foreman#9808
Solution
Add field
Related Issues
dependency SatelliteQE/nailgun#1257