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

stagingapi: carry over build state during supersede. #897

Merged
merged 1 commit into from
Sep 7, 2017

Conversation

jberry-suse
Copy link
Contributor

Allows for packages being added to rings to remain enabled after being superseded.

Likely need to test on a local OBS, build out a supersede test case (not trivial), or cross-fingers.

DO NOT ACCEPT UNTIL SOMEONE HAS HAD A CHANCE TO SEE THIS WORK.

Fixes #896.

Presumably, it is desirable to keep sub package states which is what I did.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 44.69% when pulling f8cc20e on jberry-suse:supersede-build-remain into ce3b2e1 on openSUSE:master.

@lnussel
Copy link
Member

lnussel commented Jun 19, 2017

so anyone confirmed this working?

@jberry-suse
Copy link
Contributor Author

We could either risk it, create a manual scenario, or wait for me to have time to rework testing setup so this is not a pain to test.

delete_package(self.apiurl, subprj, package, force=True, msg=msg)

for sub_prj, sub_pkg in self.get_sub_packages(package):
sub_prj = self.map_ring_package_to_subject(project, sub_pkg)
if self._supersede:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this should not needed, ring package should always be enabled to build and it's sub-package should do too, carry over build state is only apply to new package request.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean 'new in ring'? as we might have an existing package that is newly promoted to a ring, paired with a submission introducing new fixes needed too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever it's new package submission to Factory and will be promoted to a ring or an existing package update and will be promoted to a ring, Rings should not have changed until the submission merged to Factory, thus there is none from get_sub_packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought at this point is two-fold:

  1. Lets prioritize Bring up real OBS against which tests may be run #1002 (real OBS to test against) so we have some way to walk through all the possible workflows and verify since this is some fairly scary code.
  2. It may make sense for me to revisit my prototype ring command (ring: create command for making and recording ring changes and process during accept #848) and finish it as that would provide supplemental information that would make it crystal clear which packages should be enabled rather than the somewhat wishy-washy alternatives (looking at previous build settings).

@nilxam
Copy link
Contributor

nilxam commented Sep 4, 2017

I'd like to give it a try in real however staging-bot always win the game :-)
How about we just merge it and see what happened on staging-bot? it should not break things but just about changing build state, we can survived with it in case something goes wrong.

@nilxam
Copy link
Contributor

nilxam commented Sep 5, 2017

hey @jberry-suse , I tested your code, there is a bug about join(), in this change there is two arguments given to join() hence it fails to work, I think it should mean a tuple of strings ie. '/'.join([project, tar_pkg] , so I changed all those places to '/'.join([project, tar_pkg] and tried it again, looks works fine.

Allows for packages being added to rings to remain enabled after being
superseded.
@jberry-suse
Copy link
Contributor Author

Thanks for testing. I made the following changes.

diff --git a/osclib/stagingapi.py b/osclib/stagingapi.py
index 7530d4d..9c2407c 100644
--- a/osclib/stagingapi.py
+++ b/osclib/stagingapi.py
@@ -782,7 +782,7 @@ class StagingAPI(object):
         meta = ET.fromstring(''.join(meta))
         disabled = len(meta.xpath('build/disable[not(@*)]')) > 0
         if store:
-            self._package_disabled['/'.join(project, package)] = disabled
+            self._package_disabled['/'.join([project, package])] = disabled
         return disabled
 
     def create_package_container(self, project, package, disable_build=False):
@@ -1119,7 +1119,7 @@ class StagingAPI(object):
                 project = self.map_ring_package_to_subject(project, tar_pkg)
 
             if self._supersede:
-                disable_build = self._package_disabled.get('/'.join(project, tar_pkg), disable_build)
+                disable_build = self._package_disabled.get('/'.join([project, tar_pkg]), disable_build)
 
         self.create_package_container(project, tar_pkg,
                                       disable_build=disable_build)
@@ -1147,7 +1147,7 @@ class StagingAPI(object):
             if sub_prj == project:  # skip inner-project links
                 continue
             if self._supersede:
-                disable_build = self._package_disabled.get('/'.join(sub_prj, sub_pkg), False)
+                disable_build = self._package_disabled.get('/'.join([sub_prj, sub_pkg]), False)
             self.create_package_container(sub_prj, sub_pkg, disable_build=disable_build)
 
             root = ET.Element('link', package=tar_pkg, project=project)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 48.081% when pulling 00dee0d on jberry-suse:supersede-build-remain into 5900f69 on openSUSE:master.

@nilxam nilxam merged commit 36f1995 into openSUSE:master Sep 7, 2017
@jberry-suse jberry-suse deleted the supersede-build-remain branch September 14, 2017 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants