#473 WIP: Populate builds list with reused builds
Closed 3 years ago by gnaponie. Opened 4 years ago by srieger.
srieger/freshmaker master  into  master

file modified
+1 -1
@@ -88,7 +88,7 @@ 

  

          log = logging.getLogger()

          log.propagate = False

-         log.addHandler(journal.JournalHandler())

+         log.addHandler(journal.JournaldLogHandler())

      else:

          logging.basicConfig(filename=conf.log_file, level=conf.log_level,

                              format=log_format)

@@ -1,6 +1,7 @@ 

  # A generic, single database configuration.

  

  [alembic]

+ script_location = migrations

  # template used to generate migration files

  # file_template = %%(rev)s_%%(slug)s

  

@@ -0,0 +1,26 @@ 

+ """Add where reused builds are reused from

+ 

+ Revision ID: 9df9f02e6903

+ Revises: 2358b6f55f24

+ Create Date: 2020-02-03 13:48:45.665684

+ 

+ """

+ 

+ # revision identifiers, used by Alembic.

+ revision = '9df9f02e6903'

+ down_revision = '2358b6f55f24'

+ 

+ from alembic import op

+ import sqlalchemy as sa

+ 

+ 

+ def upgrade():

+     # ### commands auto generated by Alembic - please adjust! ###

we can remove these comments

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

+     # ### end Alembic commands ###

+ 

+ 

+ def downgrade():

+     # ### commands auto generated by Alembic - please adjust! ###

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

+     # ### end Alembic commands ###

file modified
+9 -3
@@ -532,11 +532,15 @@ 

  

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

  

+     # If a build has been reused, this will be the ID of the event that it came

+     # from. If the build has not been reused, this will be None

+     reused_from = db.Column(db.Integer, nullable=True)
cqi commented 4 years ago

I'm confused with this name reused_from. If it is an event ID, could you rename it to including word "event", or something else as long as the name could reflect the meaning clearly and directly?

On the other hand, I don't understand how this field could help to fulfill the AC mentioned in the card. Could you please elaborate?

+ 

      @classmethod

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

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

                 original_nvr=None, rebuilt_nvr=None,

-                rebuild_reason=0):

+                rebuild_reason=0, reused_from=None):

  

          now = datetime.utcnow()

          build = cls(
@@ -549,7 +553,8 @@ 

              build_id=build_id,

              time_submitted=now,

              dep_on=dep_on,

-             rebuild_reason=rebuild_reason

+             rebuild_reason=rebuild_reason,

+             reused_from=reused_from,

          )

          session.add(build)

          return build
@@ -671,7 +676,8 @@ 

              "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()

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

+             "reused_from": self.reused_from,

          }

  

      def get_root_dep_on(self):

file modified
+26 -1
@@ -519,7 +519,32 @@ 

          # event based on the data. Currently it generates just

          # ManualRebuildWithAdvisoryEvent.

          parser = FreshmakerManualRebuildParser()

-         db_event = _create_rebuild_event_from_request(db.session, parser, request)

+         event = parser.parse_post_data(data)
cqi commented 4 years ago

Looks this piece of code is copied from _create_rebuild_event_from_request and modified for manual rebuild event specifically, that causes duplicate code.

Can we split the _create_rebuild_event_from_request firstly for manual rebuild event and async rebuild individually, and then modify the one for manual rebuild to add the necessary code for FACTORY-5411?

+ 

+         # Store the event into database, so it gets the ID which we can return

+         # to client sending this POST request. The client can then use the ID

+         # to check for the event status.

+         db_event = models.Event.get_or_create_from_event(db.session, event)

+         if db_event:

+             db_event.requester = g.user.username

+             db_event.requested_rebuilds = " ".join(event.container_images)

+ 

+         # We need to check for successful builds, because if they are successful

+         # then they can be reused.

+         if dependent_event and dependent_event.builds:

+             for build in dependent_event.builds:

+                 if build.state == 1:

+                     build.reused_from = dependent_event.id

+                     db_event.builds.append(build)

+ 

+         if event.requester_metadata_json:

+             db_event.requester_metadata = json.dumps(event.requester_metadata_json)

+         if dependent_event:

+             dependency = db_event.add_event_dependency(db.session, dependent_event)

+             if not dependency:

+                 log.warn('Dependency between {} and {} could not be added!'.format(

+                     event.freshmaker_event_id, dependent_event.id))

+         db.session.commit()

  

          # Forward the POST data (including the msg_id of the database event we

          # added to DB) to backend using UMB messaging. Backend will then

file modified
+34
@@ -785,6 +785,40 @@ 

             'from_advisory_id')

      @patch('freshmaker.parsers.internal.manual_rebuild.time.time')

      @patch('freshmaker.models.Event.add_event_dependency')

+     def test_dependent_manual_rebuild_on_existing_event_with_builds(self, add_dependency, time,

+                                                                     from_advisory_id, publish):

+         event = models.Event.create(db.session,

+                                     "2017-00000000-0000-0000-0000-000000000003",

+                                     "105", events.TestingEvent)

+         build = models.ArtifactBuild.create(db.session, name='test-build', state=1, event=event, type=1)

+         event.builds.append(build)

+         db.session.commit()

+         time.return_value = 123

+         from_advisory_id.return_value = ErrataAdvisory(

+             105, 'name', 'RHEL_PREP', ['rpm'])

+ 

+         payload = {

+             'errata_id': 105,

+             'container_images': ['foo-1-2'],

+             'freshmaker_event_id': 1,

+         }

+         with self.test_request_context(user='root'):

+             resp = self.client.post('/api/1/builds/', json=payload, content_type='application/json')

+         data = resp.json

+         # Other fields are predictible.

+         self.assertEqual(data['requested_rebuilds'], ["foo-1-2"])

+         assert add_dependency.call_count == 1

+         assert "105" == add_dependency.call_args[0][1].search_key

+         publish.assert_called_once_with(

+             'manual.rebuild',

+             {'msg_id': 'manual_rebuild_123', u'errata_id': 105,

+              'container_images': ["foo-1-2"], 'freshmaker_event_id': 1})

+ 

+     @patch('freshmaker.messaging.publish')

+     @patch('freshmaker.parsers.internal.manual_rebuild.ErrataAdvisory.'

+            'from_advisory_id')

+     @patch('freshmaker.parsers.internal.manual_rebuild.time.time')

+     @patch('freshmaker.models.Event.add_event_dependency')

      def test_dependent_manual_rebuild_on_existing_event_no_errata_id(

          self, add_dependency, time, from_advisory_id, publish,

      ):

Relates to JIRA: FACTORY-5411

Did the tests run correctly for you? I tried (rebasing on master) and they fail. Could you please check?

If dependent_event is None, this is going to throw an exception.

What about the script for the migration? Shouldn't we have one to apply to changes in the database?

@jkaluza @cqi @qwan could one of you also review? Thanks.

@srieger Hi, could you please rebase this PR based on the latest master? Looks the codebase these changes are based on is out-of-date.

rebased onto 1b5c8a24ee363d8da339c57dced86d6dde098164

4 years ago

Sorry forgot to commit the migration file! Should be rebased now as well. The PR is a WIP because I was looking for some advice regarding the tests. It seems like they relate to the rpm package somehow, but I am confused how this relates to what I am doing. I will continue to investigate, but if anyone has any suggestions/pointers please let me know.

rebased onto 2d73e08

4 years ago

@gnaponie @cqi Would you mind looking again now that the tests are fixed? Thanks!

Looks this piece of code is copied from _create_rebuild_event_from_request and modified for manual rebuild event specifically, that causes duplicate code.

Can we split the _create_rebuild_event_from_request firstly for manual rebuild event and async rebuild individually, and then modify the one for manual rebuild to add the necessary code for FACTORY-5411?

I'm confused with this name reused_from. If it is an event ID, could you rename it to including word "event", or something else as long as the name could reflect the meaning clearly and directly?

On the other hand, I don't understand how this field could help to fulfill the AC mentioned in the card. Could you please elaborate?

The AC includes one item for changing state_reason, this PR has no code to change that. Is the AC mentioned in the card out-of-date?

@srieger is there any update for this PR?

@gnaponie Hi! Sorry - I had been waiting for more reviewers and didn't realize that people had commented. Will look at this soon :)

Looks this piece of code is copied from _create_rebuild_event_from_request and modified for manual rebuild event specifically, that causes duplicate code.
Can we split the _create_rebuild_event_from_request firstly for manual rebuild event and async rebuild individually, and then modify the one for manual rebuild to add the necessary code for FACTORY-5411?

@cqi What do you mean by split? Like make two methods or make an if-else type of thing?

I'm confused with this name reused_from. If it is an event ID, could you rename it to including word "event", or something else as long as the name could reflect the meaning clearly and directly?
On the other hand, I don't understand how this field could help to fulfill the AC mentioned in the card. Could you please elaborate?

This is the event that the builds are being reused from, so I think renaming it to be more clear is a good idea. I think the original plan with this was to eventually reference it in the rebuild reason

What's the state of this PR? Can we resolve (either merge or close) this?

Closing since Sarah's is no longer working with us and she won't probably address these changes in the future. Who will pick up this work in the future can take inspiration by this PR.

Pull-Request has been closed by gnaponie

3 years ago