-
Notifications
You must be signed in to change notification settings - Fork 95
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
Implements virtually accept the delete request #1136
Conversation
TODO:
|
TODO list is clean up, please review! |
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.
Overall, seems like a nice approach to the problems faced with delete
requests. A few areas that would benefit from refactoring and some minor wording changes.
Although the 8 commits have nice messages it would appear 4 fall in the category of fixup
as git
would call them and should be squashed into the commits that originally added the code.
- accept_command: change delreq-monitor review to the maintainer group
- accept_command: ensure sub-package's binary are empty
- accept_command: accepting delreq-monitor group review first
- accept_command: make sure project's pseudometa have the type attribute
As noted in a review comment I assume the following commit is a bonus feature implemented in this request as otherwise I would expect it to be desirable for the non-virtual handling? No change, just curious.
- accept_command: delete sub-package links before accepting the delete request
osclib/accept_command.py
Outdated
pkgs = [] | ||
filelist = self.api.get_filelist_for_package(pkgname=package, project=self.api.project, expand='1', extension='spec') | ||
for spec in filelist: | ||
pkgs.append(spec[:-5]) |
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.
Since this same sort of logic is done in #1110, perhaps extracting as another method in StagingAPI and using for both would make sense (ie a method to return list of subpackages based on spec files in main package). Might as well merge the other and refactor in this request along with all the additional usages.
osclib/accept_command.py
Outdated
|
||
for pkg in pkgs: | ||
query = {'view': 'binarylist', 'package': pkg, 'multibuild': '1'} | ||
pkg_binarylist = ET.parse(http_GET(self.api.makeurl(['build', project, '_result'], query=query))).getroot() |
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.
Without running the queries to double check this seems like the same information provided by osclib.core.package_binary_list()
. From the comment and my memory multibuild results are returned there as well. Perhaps try it and just and check if that returns an non-empty list. Otherwise the binary_list()
method is the one used by duplicate binary check.
The main difference is the details of the information returned, but you only seem to care if any binaries or not so I would expect them to be sufficient.
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.
hmm... I don't want to hardcoding arch, the binary must be removed in any arch.
osclib/accept_command.py
Outdated
pkgs = [] | ||
filelist = self.api.get_filelist_for_package(pkgname=package, project=self.api.project, expand='1', extension='spec') | ||
for spec in filelist: | ||
pkgs.append(spec[:-5]) |
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.
Another need for having a method to return list of subpackages based on spec files as noted above.
osclib/accept_command.py
Outdated
for pkg in pkgs: | ||
meta = show_package_meta(self.api.apiurl, self.api.project, pkg) | ||
meta = ''.join(meta) | ||
# Update package meta with disabling build |
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.
nit: s/with disabling/to disable/
osclib/accept_command.py
Outdated
meta = ''.join(meta) | ||
# Update package meta with disabling build | ||
self.api.create_package_container(self.api.project, pkg, meta=meta, disable_build=True) | ||
url = self.api.makeurl(['build', self.api.project], {'cmd': 'wipe', 'repository': self.api.main_repo, 'package': pkg}) |
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.
osc.core.wipebinaries()
osclib/conf.py
Outdated
@@ -44,6 +44,8 @@ | |||
'openqa': 'https://openqa.opensuse.org', | |||
'lock': 'openSUSE:%(project)s:Staging', | |||
'lock-ns': 'openSUSE', | |||
'delreq-monitor': 'factory-maintainers', | |||
'main-repo': 'standard', |
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 should likely be used in repo_checker
as well since for lack of having a proper variable I kept the hard-coded standard
. I'll update that once this is merged.
osclib/conf.py
Outdated
@@ -44,6 +44,8 @@ | |||
'openqa': 'https://openqa.opensuse.org', | |||
'lock': 'openSUSE:%(project)s:Staging', | |||
'lock-ns': 'openSUSE', | |||
'delreq-monitor': 'factory-maintainers', |
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.
I am not clear what is the purpose behind the word monitor
in the key. Seems more like a virtual delete request queue
or backlog
or pending list
. I understand the list will be monitored to determine when it is appropriate to actually accept the delete, but given this controls the virtual delete
request feature it seems like that would be more appropriate to be in the key name.
Perhaps virtual-delete-review
or somesuch as that provides a much better idea of what this is for at a glance.
osclib/stagingapi.py
Outdated
@@ -1254,7 +1263,7 @@ def list_requests_in_prj(self, project): | |||
|
|||
return list | |||
|
|||
def add_review(self, request_id, by_project=None, by_group=None, msg=None): | |||
def add_review(self, request_id, by_project=None, by_group=None, by_user=None, msg=None): |
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.
I assume this is no longer necessary since using factory-maintainers
group?
osclib/conf.py
Outdated
@@ -58,6 +60,8 @@ | |||
'openqa': 'https://openqa.opensuse.org', | |||
'lock': 'openSUSE:%(project)s:Staging', | |||
'lock-ns': 'openSUSE', | |||
'delreq-monitor': 'factory-maintainers', |
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 it make sense to use a different user/group like you originally did to avoid the continued mixing of factory
in non-factory
projects?
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 idea here was that 'the one that will have to accept it into oS:F in the end must be in factory-maintainers' anyway; and the 'last step', removing the review of the 'delreq-monitor', would happen directly in the osc staging accept
call; so definitively by a user in the factory-maintainers group.
If we take a 2nd group, it must have all the member of factory-maintainers, plus potentially more (which did not seem to realistic)
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.
So I guess this is a symptom of the larger problem that that group is also used for Leap already. As such it makes sense to use in this context, but should be considered for changing for Leap in general at some point.
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.
indeed - so fo far the group allowed to write into the product directly was small and the same set of users is used across TW and Leap (to back each other up);
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.
So perhaps masters-of-the-obs
:)
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.
That sounds more like the obs admins; happy not to be part of that (most of the times)
osclib/accept_command.py
Outdated
|
||
return True | ||
|
||
def find_virtual_accepted_requests(self, project): |
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.
Rather than duplicate all the dom walking code from find_new_requests()
it would seem to make more sense to rename that method to somethign like find_requests()
and add a parameter to indicate looking for virtual delete requests.
62d5f7d
to
1f0d075
Compare
updated, still need to test it in real. |
a34901d
to
bbe833b
Compare
Fai
|
osclib/accept_command.py
Outdated
@@ -51,6 +84,20 @@ def reset_rebuild_data(self, project): | |||
content = ET.tostring(root) | |||
http_PUT(url + '?comment=accept+command+update', data=content) | |||
|
|||
def virtually_accept_delete(self, request_id, package): | |||
self.api.add_review(request_id, by_group=self.api.delreq_reivew) |
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.
typo.. this should be self.api.delreq_review
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.
sigh...
osclib/accept_command.py
Outdated
change_request_state(self.api.apiurl, str(req['id']), 'accepted', message='Accept to %s' % self.api.project) | ||
# Check if all .spec files of the package we just accepted has a package container to build | ||
self.create_new_links(self.api.project, req['packages'][0], oldspecs) | ||
changed = True | ||
|
||
return changed | ||
|
||
def remove_obsoleted_develtag(self, project, package): | ||
xpath = { | ||
'package': "@project='%s' and devel/@project='%s' and devel/@package='%s'" % (project, project, package), |
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 removes the sub-packages which have their devel project set to the target project right? Not sure if you like, but one tweak that I checked works is:
@project='%s' and devel/@project=@project and devel/@package='%s'
to avoid adding project twice.
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.
No, it only removes the devel attribute from the packages - it does not remove the package (but as long as the devel attribute is set, accepting the delete request fails; with or without the virtual delete
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 only removes the devel tag from the package if it has set devel pacakge in the same project.
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, that's what I meant. Obviously, deleting a devel project would be a bit crazy.
@@ -14,7 +14,40 @@ def __init__(self, api): | |||
self.api = api | |||
|
|||
def find_new_requests(self, project): | |||
query = "match=state/@name='new'+and+(action/target/@project='{}'+and+action/@type='submit')".format(project) | |||
query = "match=state/@name='new'+and+action/target/@project='{}'".format(project) |
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 will now include things like change_devel
requests. Are you sure the rest of the code can handle them properly or should it explicitly look for submit or delete requests?
I had to restricted explicitly for SLE in #1121 which is the equivalent of change_devel
in Factory so it be good to make sure we do not introduce another case.
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.
any kind of request must be reviewed by human and bot, if it's state is new
then means all reviews are passed, in the past, it was merged manually(either change_devel request to Factory or add_role to SLE), this change just auto-merge those kind of request while executing accept_other_new.
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.
I understand they were reviewed, a question of can the code handle the structure of those different types of requests or does it make assumptions as the code I had to restrict did.
osclib/accept_command.py
Outdated
} | ||
collection = search(self.api.apiurl, **xpath)['package'] | ||
pkglist = [p.attrib['name'] for p in collection.findall('package')] | ||
for pkg in pkglist: |
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.
Any reason to build a list and then iterate over it rather than just looping over the original data structure?
for pkg in collection.findall('package'):
set_devel_project(self.api.apiurl, project, pkg.attrib['name'], devprj=None)
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.
just different coding style not you like...
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.
Indeed although a bit more than just style given the performance impact albeit tiny, nothing incorrect about it, just curious as typically looping twice is worse than looping once.
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.
Well, loops an extra time and allocates an entire extra array. Again tiny, but I see no upside. From readability standpoint I now have to understand the transformation from original structure to new one and keep the new on in my head while reading the loop instead of just one structure, no transformation, and access data directly the same way it is done countless other places.
osclib/accept_command.py
Outdated
message='Accept to %s' % self.api.project) | ||
self.create_new_links(self.api.project, req['package'], oldspecs) | ||
if 'type' in req and req['type'] == 'delete' and self.api.delreq_review: | ||
# Virtually accept to the delete request |
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.
nit: s/to //
Although really it now says almost exactly the same thing the method name does which isn't terribly helpful.
bbe833b
to
30eb46f
Compare
Last accept round was done with this PR merged on my local branch The package
Once the current snapshot (0928) is being published, the binaries should all be gone and the 'accepting of the delete request' should be completed (to be seen) |
30eb46f
to
27ba1fe
Compare
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.
Assuming the drop of request type filter in find_new_requests()
does not cause issues with change_devel
requests. It would seem to make sense to restrict to the types that the code actually wants to handle.
Thanks for improving the code, and overall seems like a nice solution.
27ba1fe
to
f5e7c2e
Compare
Imho, accept_other_new should really handle all 'new' requests; any incoming request on openSUSE:Factory receives a 'review' at least on factory-staging and a couple bots. at least factory-staging is a 'human review' for non-submit and non-delete requests, as those are not grouped into staging projects (and staging list / staging select ignores them) |
f5e7c2e
to
61295d0
Compare
Added an additional change to cope with 'virtual accept' in Rings. The package is deleted in theory thus should happened in Rings for real. if self.api.ring_packages.get(pkg): delete_package(self.api.apiurl, self.api.ring_packages.get(pkg), pkg, force=True, msg="Cleanup package in Rings") |
Implementation virtually accept the delete request, the delete request will be added another delreq-review review and also wipes the binary in the main project, the backend will sync 'nothing' to ToTest and Snapshot after all. Once all repository has no binary then remove the package in real.
https://progress.opensuse.org/issues/20420
Workflow:
selecting a request to a staging, the pseudometa is changed to
{id: 100, package: FOO, author: user, type: submit}
{id: 101, package: BAR, author: user, type: delete}
during accepting a delete request:
If 'type' is exists in pseudometa and delreq-monitor was set in conf.py, the delete request will through virtual accept process, the binary will be removed on standard repo. If 'type' is not exists, go through the old route make sure it will not break the current stuff.
in the later check-in round:
If it has found the virtual accepted request's package doesn't have binary on any build repo in the main project, the accept command accepts delreq-monitor review and continue accepts this request to the main project via accept_other_new().