Skip to content
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

RequestSplitter and staging selection proposals #637

Merged
merged 7 commits into from Jan 30, 2017

Conversation

@jberry-suse
Copy link
Contributor

jberry-suse commented Jan 13, 2017

While working on openSUSE:Leap:42.3 I have maintained a few local wrapper scripts to assist in some of the staging work. With the addition of #631 proposals would not include undesired requests and so I decided to build out what I had been thinking about.

The goal is threefold:

  1. replace list and adi parts with a common request filter/group code base
  2. utilize such a framework for adding filter/group capability to select command
  3. lay the groundwork for potential future work in creating higher level select strategies to fully automate the process as much as possible

I believe the first two are achieved quite well by this and much of the third.

The following is taken from the included documentation.

When using --filter-by or --group-by the xpath will be applied to the
request node as returned by OBS. Several values will supplement the
normal request node.

- ./action/source/@devel_project: the devel project for the package
- ./action/target/@ring: the ring to which the package belongs
- ./@ignored: either false or the provided message

Some useful examples:

--filter-by './action/target[starts-with(@package, "yast-"]'
--filter-by './action/source/[@devel_project="YaST:Head"]'
--filter-by './action/target[@ring="1-MinimalX"]'

--group-by='./action/source/@devel_project'
--group-by='./action/target/@ring'

Note that when using proposal mode multiple letter stagings to consider
may be provided in addition to a list of request IDs by which to filter.
A more complex example:

select --group-by='./action/source/@devel_project' A B C 123 456 789

This will separate the requests 123, 456, 789 by devel project and only
consider stagings A, B, or C, if available, for placement.

No arguments is also a valid choice and will propose all non-ignored
requests into the first available staging. Note that bootstrapped
stagings are only used when either required or no other stagings are
available.

Another useful example is placing all open requests into a specific
letter staging with:

select A

Interactive mode allows the proposal to be modified before application.

Hopefully, from that you have some ideas on how to utilize this.

The included --interactive flag provides an interactive mode for changing the proposal in a similar fashion to git rebase -i. This is rather slick and allows for easy proposal cleanup or just always use and start from scratch as it is fairly easy to move requests around how ever desired rather then coming up with a full list for manual select commands.

One note is that lxml is required in order to have more fully-featured xpath expressions. With that in mind there are obviously more possibilities than those listed in the documentation (like regular expressions). I will be playing with this to ensure everything works as this replaces quite a lot of existing functionality so hold off accepting. That said I would appreciate feedback and testing. The adi and list commands are a fair bit easier to read and understand as I see it and can be expanded to expose the additional filter/group by functionality if desired. I believe after the last round of changes there may be some things I can further strip out of those two files as well.

I made the last set of staging selections for openSUSE:Leap:42.3 using a generated proposal. :)

Have fun.

@jberry-suse jberry-suse force-pushed the jberry-suse:select-proposal branch 3 times, most recently from f20c922 to b1f378a Jan 13, 2017
@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Jan 13, 2017

A new command, perhaps inspect or rebase, that accepts a list of stagings and allows requests to be moved or removed from any of the stagings or between those would be relatively simple addition. A merge command or flag could be provided to allow merging additional requests using the same strategy used to populate a staging. For example, if YaST:Head devel project filter was used to fill staging C then a future select call could check existing stagings and merge any new requests that match the options used to fill that staging. So in that case if C was filled before yast-foo request was filed it could be automatically placed with the others. This example also maps to adi stagings which currently are dependent on when and how often the adi command is run.

Currently, when a list of stagings to consider is provided the list is merged with available stagings. For example, if A, B, and C are available and C, and D are specified only C will be considered. After thinking about the use-cases I believe dropping the merge and simply taking the input as is will be the best choice. That said having merge logic in place would probably make for a better proposal as otherwise it would consider full stagings the same as empty ones. For now then I'll leave as is.

I have a few small changes to provide more specific error output and handling. After that I'll need to fix the tests which currently fail due to missing lxml. I'd like to confirm making lxml a requirement is alright before going much further.

For those looking to try it out, be sure to set EDITOR environment variable as desired when using --interactive otherwise it will fallback to xdg-open for the yaml. For context the staging proposals look like the following:

osc staging -p openSUSE:Leap:42.3 select
all:
  bootstrap_required: true
  requests:
    450181: qemu
    450182: gdk-pixbuf
    450183: bash-completion
  staging: A

Where all is the group. So in this case all requests are placed in single group instead of by devel project or somesuch. The request lines can be moved, commented, or deleted. The staging letter can also be changed. The bootstrap_required was used when selecting a staging and is there for reference.

osc staging -p openSUSE:Leap:42.3 select --group-by='./action/source/@devel_project'
GNOME:Factory:
  bootstrap_required: false
  requests:
    450182: gdk-pixbuf
  staging: E
Virtualization:
  bootstrap_required: false
  requests:
    450181: qemu
  staging: B
shells:
  bootstrap_required: true
  requests:
    450183: bash-completion
  staging: A

Note that at the time of running this command C and D are occupied and as such qemu falls back to a bootstrapped staging even though it does not require it.

Another cool example is selecting request based on text of ignore message. So in the case where multiple requests are ignored for the same reason they can all be staged as soon as that problem is solved.

osc staging -p openSUSE:Leap:42.3 select --filter-by='contains(@ignored, "augeas-lenses")'
all:
  bootstrap_required: false
  requests:
    449500: yast2-ntp-client
  staging: E

As of now, I have to comment splitter.filter_add('@ignored="false"') in order for that to work. I'll have to decide if @ignored filters can/should be auto-detected and that filter dropped or if a flag to consider ignored requests is more appropriate. Leaning towards the latter since that seems cleaner and will allow easily select --ignored.

Or just exclude a single request from proposal --filter-by='@id!="450183"', etc...

Also note the RequestSplitter supports multiple group bys, but I'll need to figure out how to cleanly represent on command line. It will likely make sense to make some shorthand for common filter/group clauses.

From today then, no need for interactive.

$ osc staging -p openSUSE:Leap:42.3 select 
all:
  bootstrap_required: true
  requests:
    450181: qemu
    450182: gdk-pixbuf
    450183: bash-completion
  staging: A

Accept proposal? [y/n]: y
Staging A
(1/3) Adding request "450181" to project "openSUSE:Leap:42.3:Staging:A"
(2/3) Adding request "450182" to project "openSUSE:Leap:42.3:Staging:A"
(3/3) Adding request "450183" to project "openSUSE:Leap:42.3:Staging:A"
@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Jan 13, 2017

To speak a bit towards the third goal, I'd like to stack on top of this a set of strategies for creating a proposal and a higher level bit for choosing the most effective strategy given the state of stagings and requests. That way at some point in the future we might even be able to have a cronjob run select command and rebase to make the occasional adjustment.

@lnussel

This comment has been minimized.

Copy link
Member

lnussel commented Jan 17, 2017

In general this sounds pretty cool. The xpath filtering seems to be a bit too much though. I at least wouldn't be able to remember xpath queries. So IMO it would be better to provide the most common use cases pre-defined (like by devel) and leave the rest to $EDITOR.

@lnussel

This comment has been minimized.

Copy link
Member

lnussel commented Jan 17, 2017

wrt lxml it seems to be in sle so should be ok to use if really needed.

@nilxam

This comment has been minimized.

Copy link
Contributor

nilxam commented Jan 17, 2017

hmm...IMO the 3 point will not well workable for Tumbleweed staging as we always need to play "moving request" game from different staging; adi staging or backlog.

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Jan 17, 2017

@lnussel Yes, I agree the common filters/groups should have shorthand versions which I noted is something which can be added. Using the xpath under the hood makes the code much simpler, cleaner, and flexible in that it supports just about anything should we want/need it in the future. Since the lxml seems alright I'll look into the tests failing and polishing this up.

@nilxam Yeah, I imagine there will always be manual moving around, but I also would guess that packages will also be either staged correctly or staged incorrectly once which could be done and manually move those that are wrong. I figured having a bot keep track of pairings or analyze dependency tree might allow for rather solid proposals. Either way, I would expect the filter/grouping to aid in manually staging, but I understand that the Tumbleweed case is much more busy/complicated than what I typically deal with in Leap.

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Jan 25, 2017

I spent some time to ensure the edge-cases and previous functionality were all there in addition to using this branch for daily work for last couple weeks.

The previous code supported selecting to adi stagins if the full project name was used, but does not appear to have a short-hand method. Since the proposal generation wants to be able to support multiple stagings and requests being passed in at the same time this complicates differentiating the arguments. When considering Factory has special stagings like Gcc6 of PIE that break the letter rule checking the length of a staging name is not sufficient either. I noticed that the special stagings at least seem to start with a capitol letter which is handy for making them unique when compared to packages like Gcc6 and gcc6.

Given the above I wanted to achieve the following.

  • short-hand for adi projects
  • handle special stagings > 1 letter
  • allow selecting a package to a staging of the same name

In order for adi stagings to not be confused with request numbers (aside from cheating since request IDs are much larger, but could do that) they follow the form adi:\d which is consistent since the $project:Staging: prefix is removed for all short-hand entries. Arguments are checked against the list of short-hand stagings and full name projects to determine if a staging and are only used as staging once.

For example, the following work.

# assume first time seeing gcc is staging (if found in list) and second is request
select gcc gcc
# allows for merging a package into existing adi staging
select adi:14 someadipackage
select openSUSE:Leap:42.3:Staging:adi:14 gcc
# multiple stagings and requests still works

Given that special stagings are currently unique and that seems like a good practice the duplicate argument case should not normally be reached.

The proposal mode does not support adi stagings (ie multiple adi stagings or mimicing adi command, but this could be considered down the road as it isn't too much of a change).

Multiple filter-by and group-by options are now exposed to CLI.

Resolved the case of listing multiple stagings that are already filled. The assumption is if a specific list of stagings is provided they are used in proposal regardless of state. From usage this seems to make the most sense.

One the topic of short-hand for the xpaths, that seems to make sense, but should write up what common usages are (or perhaps figure out how to log usage) to determine what makes the most sense. For now the common ones can be copy/pasted from help text. All previous options are still supported like --by-develproject on adi command.

The tests have also been fixed and the one warning related to __future__.

Given the above, this seems ready to merge from my perspective. Follow-up work can be done fairly easily on top of this base.

The inner diff of changes: https://gist.github.com/jberry-suse/c0106957fac0b64453964ee96de786df

@jberry-suse jberry-suse force-pushed the jberry-suse:select-proposal branch 3 times, most recently from 5cb7460 to a4ad129 Jan 25, 2017
jberry-suse added 4 commits Jan 13, 2017
- re-implement list and adi commands using RequestSplitter
- numerous small cleanups and clarity improvements
- notably, adi now prints similar output to select when adding requests
- lxml is needed to provide more fully-featured xpath implementation
- extract_staging_short().
- prj_from_short()
- get_staging_projects_short()
@jberry-suse jberry-suse force-pushed the jberry-suse:select-proposal branch from a4ad129 to b698a7a Jan 25, 2017
@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Jan 25, 2017

As a note, I did a fair bit of the revamp work today so that negates some of my testing over the last couple weeks. As such if anyone gets the chance to use it a bit that would probably be good. Otherwise, I imagine I'll be able to use adi and select commands properly tomorrow.

@jberry-suse jberry-suse force-pushed the jberry-suse:select-proposal branch from b698a7a to 5b79718 Jan 25, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 25, 2017

Coverage Status

Coverage decreased (-1.03%) to 45.278% when pulling 5b79718 on jberry-suse:select-proposal into a2c4df2 on openSUSE:master.

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Jan 26, 2017

Still works. :)

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Jan 26, 2017

One area I was reminded of today which I would like some input. The difference between adi and listcommands in determining the devel project. Since this code replaces both with shared code I ended up taking the adi approach, but I am not sure it totally makes sense. It seems like Factory would (and currently is) making extra requests since it would first look at source project which will not have devel and then look at Factory. Seems like either the always assuming get_devel_project('openSUSE:Factory', target_package) is the way to find devel or perhaps a check to see if target_project is factory to avoid useless check devel using source project.

To be clear it seems wrong that adi and list differ in behavior now and the following example demonstrates the flow I would imagine factory follows for adi command.

graphics/blender -> openSUSE:Factory/blender
  • devel(graphics, blender) = None
  • devel(openSUSE:Factory, blender) = graphics
devel_project = self.api.get_devel_project(source_project, source_package)
# try target pacakge in Factory
# this is a bit against Leap development in case submissions is from Update,
# or any other project than Factory
if devel_project is None and self.api.project.startswith('openSUSE:'):
    devel_project = self.api.get_devel_project('openSUSE:Factory', target_package)
if devel_project is not None:
    source_project = devel_project

vs list

  • devel(openSUSE:Factory, graphics)
devel = self.api.get_devel_project("openSUSE:Factory", target_package)

Both implementations:

Either way I imagine it works fine, just seems like needless requests for Factory and not sure what cases benefit from checking source project.

@lnussel lnussel merged commit 5022deb into openSUSE:master Jan 30, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jberry-suse jberry-suse deleted the jberry-suse:select-proposal branch Feb 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.