-
Notifications
You must be signed in to change notification settings - Fork 285
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
CP-46944: Update yum plugins to dnf plugins #5526
Conversation
Signed-off-by: Lin Liu <lin.liu@citrix.com>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## feature/xs9 #5526 +/- ##
=============================================
+ Coverage 51.8% 53.7% +1.9%
=============================================
Files 19 23 +4
Lines 2586 2694 +108
=============================================
+ Hits 1341 1449 +108
Misses 1245 1245
Flags with carried forward coverage won't be shown. Click here to find out more. |
970632a
to
eb59b89
Compare
Investigate the python CI failures, It is somehow NOT scanned, try to re-open a new one. |
dde0d5a
to
2b1133f
Compare
github CI has issues with - ubuntu-22.04 container - python11 - python3-dnf Instead of patching the upstreams, we just provide a stub for the fix Signed-off-by: Lin Liu <lin.liu@citrix.com>
2b1133f
to
09a2fdc
Compare
4664ef0
to
4dbc807
Compare
def test_repo_without_access_token(self, mock_grabber): | ||
"""If repo has not accestoken, it should not be blocked""" | ||
mock_repo = _mock_repo() | ||
accesstoken.AccessToken(mock_repo.base, MagicMock()).config() |
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.
Need a assert_not_called
?
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.
As comments, the target is to test repo without accesstoken still work, so this case does not care whether it is called or not,
but it has no bad to put a no called there anyway.
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.
Edit: just updated to follow the suggestion.
repo = self.base.repos[repo_name] | ||
|
||
if len(repo.baseurl) > 0 and repo.baseurl[0].startswith("http://127.0.0.1") \ | ||
and repo.ptoken: |
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.
This is a newly added config item for dnf? May I please know the reason to add this?
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.
Would this require changes on xapi code which populate the repo configurations?
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.
do you mean ptoken
?
No, this is an optional configure item for the repo, to decide whether we should enable pool token for a repo.
it is used by yum, and I patched dnf to support this.
No extra xapi code change is required for this, on the other side, dnf is pathed to support this as xapi use it.
def _mock_repo(a_token=None, p_token=None, baseurl=None): | ||
mock_repo = MagicMock() | ||
mock_repo.accesstoken = a_token | ||
mock_repo.ptoken = p_token |
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.
If there is a good reason to add this "ptoken" config in dnf repo, then what information it would contain?
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.
a boolean to indicate whether enable pool token for this repo. refer to https://github.com/xapi-project/xen-api/blob/master/ocaml/xapi/repository_helpers.ml#L432
|
||
class Ptoken(dnf.Plugin): | ||
"""ptoken plugin class""" | ||
name = "ptoken" |
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.
It seems this "name" is required by dnf plugin?
(Would be better to keep a consistent formating style for both plugins, e.g. the blank lines here.)
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.
Yes, it just follow the dnf plugin protocols, find this in the comments:
refer to https://dnf.readthedocs.io/en/latest/api_plugins.html
Signed-off-by: Lin Liu <lin.liu@citrix.com>
4dbc807
to
266edbc
Compare
pytype_reporter extracted 50 problem reports from pytype output. You can check the results of the job here |
XS9 does not have yum command, and yum plugin is used to add HTTP headers automatically for yum commands when
This is updating the yum plugins to corresponding DNF plugins, and enable dnf with the same functionality.
Test done:
Manul test:
print("set ptoken: ", f'cookie:{secret}')
print("set accesstoken: ", access_token, "tokenid: ", referer)
call path:
sync_updates --> set_available_updates -> get_hosts_updates -> http_get_host_updates_in_json --> Xapi_http.http_request Constants.get_host_updates_uri