#380 Add ArtifactBuild.rebuild_reason column.
Merged 4 years ago by jkaluza. Opened 4 years ago by jkaluza.
jkaluza/freshmaker rebuild-reason  into  master

@@ -258,7 +258,8 @@ 

  

      def record_build(self, event, name, artifact_type,

                       build_id=None, dep_on=None, state=None,

-                      original_nvr=None, rebuilt_nvr=None):

+                      original_nvr=None, rebuilt_nvr=None,

+                      rebuild_reason=0):

          """

          Record build in db.

  
@@ -273,6 +274,8 @@ 

              ``ArtifactBuildState.BUILD``.

          :param original_nvr: The original NVR of artifact.

          :param rebuilt_nvr: The NVR of newly rebuilt artifact.

+         :param rebuild_reason: The reason why this artifact is included in

+             this event.

          :return: recorded build.

          :rtype: ArtifactBuild.

          """
@@ -285,7 +288,8 @@ 

          build = models.ArtifactBuild.create(db.session, ev, name,

                                              artifact_type.name.lower(),

                                              build_id, dep_on, state,

-                                             original_nvr, rebuilt_nvr)

+                                             original_nvr, rebuilt_nvr,

+                                             rebuild_reason)

  

          db.session.commit()

          return build

@@ -32,7 +32,8 @@ 

  from freshmaker.lightblue import LightBlue

  from freshmaker.pulp import Pulp

  from freshmaker.errata import Errata

- from freshmaker.types import ArtifactType, ArtifactBuildState, EventState

+ from freshmaker.types import (

+     ArtifactType, ArtifactBuildState, EventState, RebuildReason)

  from freshmaker.models import Event, Compose

  from freshmaker.utils import get_rebuilt_nvr

  
@@ -243,12 +244,22 @@ 

                  rebuilt_nvr = get_rebuilt_nvr(ArtifactType.IMAGE.value, nvr)

                  image_name = koji.parse_NVR(image["brew"]["build"])["name"]

  

+                 # Only released images are considered as directly affected for

+                 # rebuild. If some image is not in the latest released version and

+                 # it is included in a rebuild, it must be just a dependency of

+                 # other image.

+                 if "latest_released" in image:

+                     rebuild_reason = RebuildReason.DIRECTLY_AFFECTED.value

+                 else:

+                     rebuild_reason = RebuildReason.DEPENDENCY.value

+ 

                  build = self.record_build(

                      event, image_name, ArtifactType.IMAGE,

                      dep_on=dep_on,

                      state=ArtifactBuildState.PLANNED.value,

                      original_nvr=nvr,

-                     rebuilt_nvr=rebuilt_nvr)

+                     rebuilt_nvr=rebuilt_nvr,

+                     rebuild_reason=rebuild_reason)

  

                  # Set context to particular build so logging shows this build

                  # in case of error.

@@ -0,0 +1,22 @@ 

+ """Add rebuild_reason column.

+ 

+ Revision ID: fbc2eac9bfa5

+ Revises: 5bdd5566615a

+ Create Date: 2019-04-23 10:20:00.766108

+ 

+ """

+ 

+ # revision identifiers, used by Alembic.

+ revision = 'fbc2eac9bfa5'

+ down_revision = '5bdd5566615a'

+ 

+ from alembic import op

+ import sqlalchemy as sa

+ 

+ 

+ def upgrade():

+     op.add_column('artifact_builds', sa.Column('rebuild_reason', sa.Integer(), nullable=True))

+ 

+ 

+ def downgrade():

+     op.drop_column('artifact_builds', 'rebuild_reason')

file modified
+11 -3
@@ -37,7 +37,8 @@ 

  from freshmaker import db, log

  from freshmaker import messaging

  from freshmaker.utils import get_url_for, krb_context

- from freshmaker.types import ArtifactType, ArtifactBuildState, EventState

+ from freshmaker.types import (ArtifactType, ArtifactBuildState, EventState,

+                               RebuildReason)

  from freshmaker.events import (

      MBSModuleStateChangeEvent, GitModuleMetadataChangeEvent,

      GitRPMSpecChangeEvent, TestingEvent, GitDockerfileChangeEvent,
@@ -473,12 +474,17 @@ 

      # Build args in json format.

      build_args = db.Column(db.String, nullable=True)

  

+     # The reason why this artifact is rebuilt. Set according to

+     # `freshmaker.types.RebuildReason`.

+     rebuild_reason = db.Column(db.Integer, nullable=True)

+ 

      composes = db.relationship('ArtifactBuildCompose', back_populates='build')

  

      @classmethod

      def create(cls, session, event, name, type,

                 build_id=None, dep_on=None, state=None,

-                original_nvr=None, rebuilt_nvr=None):

+                original_nvr=None, rebuilt_nvr=None,

+                rebuild_reason=0):

  

          now = datetime.utcnow()

          build = cls(
@@ -490,7 +496,8 @@ 

              state=state or ArtifactBuildState.BUILD.value,

              build_id=build_id,

              time_submitted=now,

-             dep_on=dep_on

+             dep_on=dep_on,

+             rebuild_reason=rebuild_reason

          )

          session.add(build)

          return build
@@ -610,6 +617,7 @@ 

              "url": build_url,

              "build_args": build_args,

              "odcs_composes": [rel.compose.odcs_compose_id for rel in self.composes],

+             "rebuild_reason": RebuildReason(self.rebuild_reason or 0).name.lower()

          }

  

      def get_root_dep_on(self):

file modified
+11
@@ -90,3 +90,14 @@ 

      SKIPPED = 4

      # handling of the event has been canceled (also canceling builds etc.)

      CANCELED = 5

+ 

+ 

+ class RebuildReason(Enum):

+     # Rebuild reason is unknown - mainly to have some value for old

+     # ArtifactBuilds.

+     UNKNOWN = 0

+     # The artifact is directly rebuilt, because it is directly affected by some

+     # CVE, RPM update, ...

+     DIRECTLY_AFFECTED = 1

+     # The artifact is rebuilt, because it is dependency of other artifact.

+     DEPENDENCY = 2

@@ -34,7 +34,8 @@ 

  from freshmaker.handlers.internal import UpdateDBOnAdvisoryChange

  from freshmaker.lightblue import ContainerImage

  from freshmaker.models import Event, ArtifactBuild, EVENT_TYPES

- from freshmaker.types import ArtifactBuildState, ArtifactType, EventState

+ from freshmaker.types import (

+     ArtifactBuildState, ArtifactType, EventState, RebuildReason)

  from tests import helpers

  

  
@@ -274,10 +275,11 @@ 

          super(TestBatches, self).tearDown()

          self.patcher.unpatch_all()

  

-     def _mock_build(self, build, parent=None, error=None):

+     def _mock_build(

+             self, build, parent=None, error=None, **kwargs):

          if parent:

              parent = {"brew": {"build": parent + "-1-1.25"}}

-         return ContainerImage({

+         d = {
yashn commented 4 years ago

Optional: Can I please suggest against using alphabets as variable names. Instead we should have a meaningful variable name. :-)

              'brew': {'build': build + "-1-1.25"},

              'repository': build + '_repo',

              'parsed_data': {
@@ -297,7 +299,9 @@ 

              "arches": "x86_64",

              "odcs_compose_ids": [10, 11],

              "published": False,

-         })

+         }

+         d.update(kwargs)

+         return ContainerImage(d)

  

      @patch('freshmaker.odcsclient.create_odcs_client')

      def test_batches_records(self, create_odcs_client):
@@ -330,8 +334,8 @@ 

                     [self._mock_build("child1_parent2", "child1_parent3"),

                      self._mock_build("child2_parent1", "child2_parent2")],

                     [self._mock_build("child1_parent1", "child1_parent2", error="Fail"),

-                     self._mock_build("child2", "child2_parent1")],

-                    [self._mock_build("child1", "child1_parent1")]]

+                     self._mock_build("child2", "child2_parent1", latest_released=True)],

+                    [self._mock_build("child1", "child1_parent1", latest_released=True)]]

  

          # Flat list of images from batches with brew build id as a key.

          images = {}
@@ -362,6 +366,11 @@ 

              else:

                  self.assertEqual(build.dep_on, None)

  

+             if build.name in ["child1", "child2"]:

+                 self.assertEqual(build.rebuild_reason, RebuildReason.DIRECTLY_AFFECTED.value)

+             else:

+                 self.assertEqual(build.rebuild_reason, RebuildReason.DEPENDENCY.value)

+ 

              args = json.loads(build.build_args)

              self.assertEqual(args["repository"], build.name + "_repo")

              self.assertEqual(args["commit"], build.name + "_123")

file modified
+7 -3
@@ -24,7 +24,7 @@ 

  from freshmaker.models import ArtifactBuild, ArtifactType

  from freshmaker.models import Event, EventState, EVENT_TYPES, EventDependency

  from freshmaker.models import Compose, ArtifactBuildCompose

- from freshmaker.types import ArtifactBuildState

+ from freshmaker.types import ArtifactBuildState, RebuildReason

  from freshmaker.events import ErrataAdvisoryRPMsSignedEvent

  from tests import helpers

  
@@ -41,8 +41,10 @@ 

  

      def test_creating_event_and_builds(self):

          event = Event.create(db.session, "test_msg_id", "RHSA-2017-284", events.TestingEvent)

-         build = ArtifactBuild.create(db.session, event, "ed", "module", 1234)

-         ArtifactBuild.create(db.session, event, "mksh", "module", 1235, build)

+         build = ArtifactBuild.create(db.session, event, "ed", "module", 1234,

+                                      rebuild_reason=RebuildReason.DIRECTLY_AFFECTED.value)

+         ArtifactBuild.create(db.session, event, "mksh", "module", 1235, build,

+                              rebuild_reason=RebuildReason.DEPENDENCY.value)

          db.session.commit()

          db.session.expire_all()

  
@@ -57,12 +59,14 @@ 

          self.assertEqual(e.builds[0].state, 0)

          self.assertEqual(e.builds[0].build_id, 1234)

          self.assertEqual(e.builds[0].dep_on, None)

+         self.assertEqual(e.builds[0].rebuild_reason, RebuildReason.DIRECTLY_AFFECTED.value)

  

          self.assertEqual(e.builds[1].name, "mksh")

          self.assertEqual(e.builds[1].type, 2)

          self.assertEqual(e.builds[1].state, 0)

          self.assertEqual(e.builds[1].build_id, 1235)

          self.assertEqual(e.builds[1].dep_on.name, "ed")

+         self.assertEqual(e.builds[1].rebuild_reason, RebuildReason.DEPENDENCY.value)

  

      def test_get_root_dep_on(self):

          event = Event.create(db.session, "test_msg_id", "test", events.TestingEvent)

file modified
+1
@@ -139,6 +139,7 @@ 

          self.assertEqual(data['event_id'], 1)

          self.assertEqual(data['build_id'], 1234)

          self.assertEqual(data['build_args'], {"key": "value"})

+         self.assertEqual(data['rebuild_reason'], "unknown")

  

      def test_query_builds(self):

          resp = self.client.get('/api/1/builds/')

This new column shows the reason why the artifact is rebuilt. This
is useful to decide if the artifact is directly affected by some
change which triggered the rebuild, or if it is pulled-in just
as a dependency to rebuild other artifact.

Optional: Can I please suggest against using alphabets as variable names. Instead we should have a meaningful variable name. :-)

Commit 16938c9 fixes this pull-request

Pull-Request has been merged by jkaluza

4 years ago

Pull-Request has been merged by jkaluza

4 years ago