PR#1084: frontend: non-working SRPM builder-live.log.gz link - copr/copr

copr / copr

#1084 frontend: non-working SRPM builder-live.log.gz link

Merged 4 years ago by praiskup. Opened 4 years ago by schlupov.

copr/ schlupov/copr non_working_log into master

frontend: non-working SRPM builder-live.log.gz link

Silvie Chlupova • 4 years ago

c444445

frontend/coprs_frontend/alembic/schema/versions/745250baedaf_new_column_resubmitted_from_id.py

file added

+20

		`@@ -0,0 +1,20 @@`
		`+ """`
		`+ new column resubmitted_from_id`
		`+`
		`+ Revision ID: 745250baedaf`
		`+ Revises: 0dbdd06fb850`
		`+ Create Date: 2019-11-11 11:15:05.399008`
		`+ """`
		`+`
		`+ import sqlalchemy as sa`
		`+ from alembic import op`
		`+`
		`+`
		`+ revision = '745250baedaf'`
		`+ down_revision = '0dbdd06fb850'`
		`+`
		`+ def upgrade():`
		`+ op.add_column('build', sa.Column('resubmitted_from_id', sa.Integer(), nullable=True))`
		`+`
		`+ def downgrade():`
		`+ op.drop_column('build', 'resubmitted_from_id')`

frontend/coprs_frontend/coprs/logic/builds_logic.py

file modified

		`@@ -521,6 +521,8 @@`
		`srpm_url=source_build.srpm_url, copr_dirname=source_build.copr_dir.name, **build_options)`
		`build.package_id = source_build.package_id`
		`build.pkg_version = source_build.pkg_version`
		`+ build.resubmitted_from_id = source_build.id`
		`+`
		`return build`

		`@classmethod`

frontend/coprs_frontend/coprs/models.py

file modified

+13

		`@@ -874,6 +874,11 @@`
		`# used by webhook builds; e.g. github.com:praiskup, or pagure.io:jdoe`
		`submitted_by = db.Column(db.Text)`

		`+ # if a build was resubmitted from another build, this column will contain the original build id`
		`+ # the original build id is not here as a foreign key because the original build can be deleted so we can lost`
		`+ # the info that the build was resubmitted`
		`+ resubmitted_from_id = db.Column(db.Integer)`
		`+`
		`@property`
		`def user_name(self):`
		`return self.user.name`
		`@@ -1159,6 +1164,14 @@`

		`return '{0}--{1}'.format(self.copr.full_name, submitter)`

		`+ @property`
		`+ def resubmitted_from(self):`
		`+ return Build.query.filter(Build.id == self.resubmitted_from_id).first()`
		`+`
		`+ @property`
		`+ def source_is_uploaded(self):`
		`+ return self.source_type == helpers.BuildSourceEnum('upload')`
		`+`

		`class DistGitBranch(db.Model, helpers.Serializer):`
		`"""`

frontend/coprs_frontend/coprs/templates/coprs/detail/build.html

file modified

+17 -7

		`@@ -164,16 +164,26 @@`
		`<dl class="dl-horizontal">`
		`{{ describe_failure(build) }}`
		`<dt>SRPM build log:</dt>`
		`- {% if build.source_status \| state_from_num in ['pending', 'starting'] %}`
		`- <dd> Build has not started yet </dd>`
		`+ {% if build.source_status \| state_from_num in ['pending', 'starting'] %}`
		`+ <dd> Build has not started yet</dd>`
		`+ {% else %}`
		`+ {% if build.resubmitted_from and build.source_is_uploaded %}`
		`+ <dd>Build resubmitted from build`
		`+ {% if build.resubmitted_from %}`
		`+ <a href="{{ copr_url('coprs_ns.copr_build', build.copr, build_id=build.resubmitted_from.id) }}">#{{ build.resubmitted_from.id }}</a>`
		`+ {% else %}`
		`+ #{{ build.resubmitted_from_id }}`
		`+ {% endif %}`
		`+ </dd>`
		`{% else %}`
		`- {% for url in build.get_import_log_urls(g.user.admin) %}`
		`- <dd>`
		`- <a href="{{ url }}">{{ url \| basename }}</a>`
		`+ {% for url in build.get_import_log_urls(g.user.admin) %}`
		`+ <dd>`
		`+ <a href="{{ url }}">{{ url \| basename }}</a>`
		`{{ "," if not loop.last }}`
		`- </dd>`
		`- {% endfor %}`
		`+ </dd>`
		`+ {% endfor %}`
		`{% endif %}`
		`+ {% endif %}`
		`<dt> Built Packages:</dt>`
		`{% if build.built_packages %}`
		`{% for pkg in build.built_packages.split("\n"): %}`

schlupov commented 4 years ago

Fixes: #1011

schlupov commented 4 years ago

This is how some user can see it

frostyx commented on line 46 of frontend/coprs_frontend/alembic/schema/versions/cec443fcae7b_new_column_resubmit.py 4 years ago

This migration looks absolutely insane. it can't be right :-)
You need to pick only the actions related to your models.py change and remove the rest.

frostyx commented on line 7 of frontend/coprs_frontend/coprs/models.py 4 years ago

If the column is supposed to contain IDs, then I would call it e.g. resubmitted_from_id, resubmit_id or something containing _id on its end.

Also, I would like to see an appropriate db.relationship(...), returning the original build object.

frostyx commented on line 17 of frontend/coprs_frontend/coprs/templates/coprs/detail/build.html 4 years ago

I would add a hash symbol in front of the build ID. Also, it could be click-able.

frostyx commented on line 16 of frontend/coprs_frontend/coprs/templates/coprs/detail/build.html 4 years ago

Shouldn't this condition rather be over the for loop instead of inside it? The loop would be in the else clause ... or am I wrong?

rebased onto b032e79e7bc63f82e63eee1fdcb57e254da3c4dd

4 years ago

schlupov commented 4 years ago

If the original build still exists the original build id is clickable.

praiskup commented on line 31 of frontend/coprs_frontend/coprs/views/coprs_ns/coprs_builds.py 4 years ago

Can we avoid this part, and add Build.resubmitted_from property to models? (returning the build the particular build was resubmitted from, integer (when deleted) or None if not available)?

praiskup commented on line 11 of frontend/coprs_frontend/coprs/templates/coprs/detail/build.html 4 years ago

The {{ resubmit }} special argument can be avoided by something like url_for...

rebased onto 33685671b8813a5fbdef03dbeb260878f7175cbc

4 years ago

rebased onto 6cc0ec3f59620c3772ae0add792b6892edc67c3b

4 years ago

rebased onto 7f21e40df739844b693c3d39d34a162ef017c335

4 years ago

rebased onto 9ddfbfaf3b2d59ad7b26ca4632b0108da52c491a

4 years ago

praiskup commented 4 years ago

frostyx commented on line 32 of frontend/coprs_frontend/coprs/models.py 4 years ago

AFAIK .first() returns either an object or None in opposite to .one() which raises an exception. Therefore we should be able to throw away the try-except block

frostyx commented on line 28 of frontend/coprs_frontend/coprs/models.py 4 years ago

The original_build is an ambiguous term, I find the resubmitted_from suggested by @praiskup to be a better name, but it doesn't really matter to me that much.

frostyx commented on line 6 of frontend/coprs_frontend/coprs/logic/builds_logic.py 4 years ago

Alternative idea ... wouldn't it be better to set this property for all resubmitted builds? It makes much more sense to me. And then, have this condition somewhere in the view/template?

praiskup commented on line 6 of frontend/coprs_frontend/coprs/logic/builds_logic.py 4 years ago

+1, btw. you know that I view the fact that we have different behavior for uploads vs. other methods here a bit ... I think all resubmitted builds should be done from our dist-git

Edited 4 years ago by praiskup

praiskup commented 4 years ago

Somewhat complementary proposal I yesterday mentioned to @schlupov is that we could for example stop showing builder-live.log link for all uploaded builds (not only resubmitted), because those are pretty much useless anyways.

praiskup commented 4 years ago

But since we now have the resubmitted_id property, perhaps we could only hide the builder-live.log links for "old" builds, ie those which are done before this fix gets released.

rebased onto 018f440c741d6a6fba082a9f8fbff26c3e8f913c

4 years ago

rebased onto 77dd58f93cd8ca551d34b4eb5a4f49c5ce649d0c

4 years ago

schlupov commented 4 years ago

I changed that property name to resubmitted_from.

Edited 4 years ago by schlupov

rebased onto 5be7da7c1a02cd3f60499f144cf72738bfcdb36f

4 years ago

rebased onto 5012255a9a0b08d62569220185faf1fd64321a75

4 years ago

frostyx commented 4 years ago

however I'm not sure that .one() helps us.

I am not saying, that .one() helps us, I am saying that .first() returns either an object or None, so IMHO it shouldn't need a try-except block.

frostyx commented 4 years ago

Also, what do you think about the "wouldn't it be better to set this property for all resubmitted builds? " idea?

schlupov commented 4 years ago

I don't think it is needed to set this property to every resubmitted build, only builds from upload have the different behavior (what is unfortunately not expected behavior).

frostyx commented 4 years ago

only builds from upload have the different behavior ...

True, but only in this particular use-case. What if we need to do something for all resubmitted builds in the future.

praiskup commented on line 6 of frontend/coprs_frontend/coprs/models.py 4 years ago

Yes, please drop part of this comment related to "upload-only".

praiskup commented 4 years ago

I still think we should rename it from resubmitted_id to resubmitted_from_id, and even if never planned to use it for other logic than this code - per it's name - we should store this attribute for every resubmitted build.

Edited 4 years ago by praiskup