Simplify logs #635

Zlopez · 2018-10-12T10:07:50Z

This PR introduces simple status for every project instead of current robust log table, which is not really needed (all these log messages are sent through fedmsg anyway).
This PR is split to two commits.

First commit is dropping old logs table and removing every reference to it.

Second commit is introducing new flag on project which contains current update status. Current Logs section on project page is changed to Status section which is showing the update_status information:

None - no version retrieved
True - version retrieved successfully
False - failure when obtaining version

After merge of #633 I will add a new column Last check time to above table.

codecov-io · 2018-10-12T10:10:39Z

Codecov Report

Merging #635 into master will increase coverage by 0.11%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master     #635      +/-   ##
==========================================
+ Coverage    95.7%   95.82%   +0.11%     
==========================================
  Files          57       57              
  Lines        2889     2826      -63     
  Branches      398      386      -12     
==========================================
- Hits         2765     2708      -57     
+ Misses         86       77       -9     
- Partials       38       41       +3

Impacted Files	Coverage Δ
anitya/admin.py	`91.69% <ø> (+2.57%)`	⬆️
anitya/db/models.py	`100% <100%> (ø)`	⬆️
anitya/lib/utilities.py	`96.87% <100%> (-1.78%)`	⬇️
anitya/ui.py	`98.48% <86.66%> (-0.57%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44e7fb7...94fb978. Read the comment docs.

jeremycline · 2018-10-15T15:04:36Z

anitya/db/models.py

@@ -338,6 +248,7 @@ class Project(Base):

    latest_version = sa.Column(sa.String(50))
    logs = sa.Column(sa.Text)
+    update_status = sa.Column(sa.Boolean, default=None)


I imagine this will be something that is queried on, so it should probably have an index. Also, a nit, but the name is a bit confusing as a boolean: "update_status = False" doesn't mean much to me. "update_successful" or similar would be clearer. Does it need to be nullable?

One thing you could do is make this column an integer and store the HTTP status code. If things like DNS or TLS validation fail you could store a constant you define. Then you don't need the logs column here either.

My idea was to use the update_status in this way:

if None than update wasn't done yet - this should only happen on new project

if True update was successful - this should be set if project was updated successfully, even if there is no new version retrieved

if False update failed - this should be set if project check for new version failed for any reason

project.logs should contain error encountered when retrieving the version.

I wanted to name it update instead of update_status, but there is already method named update. But this could be easily changed to update_successful.

I don't think the HTTP status is good indicator. Most of the checks are failing before or after HTTP request is made. Like Project is not correctly set-up or Can't parse new version succesfully. These are the most frequent errors that raise AnityaException without any HTTP status code. I can probably map them to some specific status code and create a mapping for status codes to status messages.

What do you think about this?

Hmm. I guess there are more possible errors than would be terribly fun to enumerate, and logs isn't indexed there's not a huge return on investment.

The only thing I'd think about is whether or not you want this column and also a column to track the last time a project was checked (so when the cron job runs you sort by oldest) or if those could be combined in some way.

This is only slightly related, but it doesn't make sense to me for a project to be created and not checked (I always check the "check now" box). Maybe we should force a check before creating the project. This way we will always report to the user their settings don't work and we don't have to handle the "project is created, but not checked" case.

Oh, and whatever you decide on it needs a docblock entry.

The column containing last time the project was checked is already implemented here #633.

But good idea about the automatic check of project. In this case, I just set the update_status, maybe I rename it to update_successful, to False until the check is actually successful.

Still it will be better to actually have some button test on create project site, that will try to check for new version, before commiting project to database and even better will be to do this also automatically in backend before commiting project.

Oh, I see I forgot to update docstring of Project class.

Main issue with automatic check right now is that you need the project to exist in the database before check. This is the reason, why you can't do test check when creating the project.

Zlopez · 2018-10-17T12:52:37Z

I did a rebase with master and renamed update_status to check_successful.
Also I updated Project docstring.

jeremycline

This needs some tests, but generally looks okay. I wonder, do you want to keep the /logs route and just change the query to grab them off the project?

Zlopez · 2018-10-18T07:15:16Z

@jeremycline
This is a good idea I will do it.

About the tests, there isn't much code added and the diff hit 75% is because one line was changed in the code that isn't covered by any tests, but this is fixed here #637

Zlopez · 2018-10-18T11:54:01Z

I added back the /logs page. It's now looking like this:

But it's now using updated_on field instead of last_check, because last_check is part of #633. So it will be probably better to merge #633 first and do small changes to this PR after.

Zlopez · 2018-12-07T18:45:36Z

I fixed the conflicts, failing tests and added last_check information to project and logs page.

I think it's ready for merge. \o/

jeremycline · 2018-12-12T20:53:02Z

anitya/db/migrations/versions/1324a36b9f69_add_updated_check_to_project.py

@@ -0,0 +1,42 @@
+"""Add updated check to project


Do you have a particular reason for not combining this with the migration that drops the logs table?

Not any particular reason, I will merge them together.

jeremycline · 2018-12-12T20:56:52Z

anitya/db/models.py

@@ -549,6 +465,36 @@ def by_distro(cls, session, distro, page=None, count=False):
        else:
            return query.all()

+    @classmethod
+    def by_last_check(cls, session, page=None, count=False):


This really belongs on a custom query class. The interface then becomes: Project.query.by_last_check which should return the query object, so you can then do Project.query.by_last_check().count(). I know there are existing class methods, but that's not a normal SQLAlchemy pattern.

It makes no sense to accept the session as a parameter in any case since you're just handing it a global variable.

Ok, I will move it to query class. I need to do a refactoring of the other models later.

At the end I decided to remove it altogether. It's called only in one place and it's basically alias for Project.query.order_by(Project.last_check.desc()). The second reason for me to decide to remove it, is that I wasn't able to call order_by in separate source file without creating circle dependency.
In anitya.db.models I needed to import anitya.db.queries to replace query property of Project model and in the anitya.db.queries I needed to import anitya.db.models to be able to call order_by(Project.last_check.desc()).

jeremycline · 2018-12-12T21:00:32Z

anitya/tests/db/test_models.py

@@ -516,6 +549,7 @@ def test_init_distro(self):
        self.assertEqual(distros[0].name, 'Debian')
        self.assertEqual(distros[1].name, 'Fedora')

+    # TODO: Split to separate tests


I recommend filing a ticket in your issue tracker over leaving a # TODO comment.

I created a new issue for this #680 and I will remove the TODO comment.

Zlopez · 2018-12-13T14:00:57Z

This pull request fixes 2 alerts when merging dbc6d16 into 1385598 - view on LGTM.com

fixed alerts:

2 for Conflicting HTML element attributes

Comment posted by LGTM.com

This commit is introduces simple status for every project instead of current robust log table. It drops the current log table and adds check_successfull flag to project. The frontend page for logs is refactored to work with this change. This change should make working with logs simpler and remove unwanted table from database. Signed-off-by: Michal Konečný <[email protected]>

Zlopez · 2019-01-07T14:11:19Z

I did a rebase and squashed and reworded commits.

Zlopez · 2019-01-07T14:35:28Z

This pull request fixes 2 alerts when merging 94fb978 into 44e7fb7 - view on LGTM.com

fixed alerts:

2 for Conflicting HTML element attributes

Comment posted by LGTM.com

Zlopez · 2019-01-08T09:15:36Z

I'm merging this manually, because I addressed all the comments made by @jeremycline and this is the last PR blocking release of 0.14.0.

jeremycline reviewed Oct 15, 2018

View reviewed changes

Zlopez force-pushed the drop_logs branch from 58ba6e6 to acb526e Compare October 17, 2018 12:51

jeremycline reviewed Oct 17, 2018

View reviewed changes

Zlopez force-pushed the drop_logs branch from 381d66e to d2908cd Compare December 7, 2018 18:43

jeremycline reviewed Dec 12, 2018

View reviewed changes

jeremycline mentioned this pull request Dec 12, 2018

WIP: Initial message schema for Anitya #570

Merged

Zlopez force-pushed the drop_logs branch from d2908cd to dbc6d16 Compare December 13, 2018 13:50

Zlopez mentioned this pull request Dec 30, 2018

New version of Solr not detected #683

Closed

Zlopez mentioned this pull request Jan 7, 2019

logs doesn't show all package events #688

Closed

Zlopez force-pushed the drop_logs branch from dbc6d16 to 94fb978 Compare January 7, 2019 14:10

Zlopez merged commit f76862b into fedora-infra:master Jan 8, 2019

Zlopez deleted the drop_logs branch January 8, 2019 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify logs #635

Simplify logs #635

Zlopez commented Oct 12, 2018

codecov-io commented Oct 12, 2018 •

edited

Loading

jeremycline Oct 15, 2018

Zlopez Oct 16, 2018

jeremycline Oct 16, 2018

Zlopez Oct 16, 2018

Zlopez Oct 17, 2018

Zlopez commented Oct 17, 2018

jeremycline left a comment

Zlopez commented Oct 18, 2018 •

edited

Loading

Zlopez commented Oct 18, 2018

Zlopez commented Dec 7, 2018

jeremycline Dec 12, 2018

Zlopez Dec 13, 2018

jeremycline Dec 12, 2018

Zlopez Dec 13, 2018 •

edited

Loading

Zlopez Dec 13, 2018

jeremycline Dec 12, 2018

Zlopez Dec 13, 2018

Zlopez commented Dec 13, 2018

Zlopez commented Jan 7, 2019

Zlopez commented Jan 7, 2019

Zlopez commented Jan 8, 2019

Simplify logs #635

Simplify logs #635

Conversation

Zlopez commented Oct 12, 2018

codecov-io commented Oct 12, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zlopez commented Oct 17, 2018

jeremycline left a comment

Choose a reason for hiding this comment

Zlopez commented Oct 18, 2018 • edited Loading

Zlopez commented Oct 18, 2018

Zlopez commented Dec 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zlopez Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zlopez commented Dec 13, 2018

Zlopez commented Jan 7, 2019

Zlopez commented Jan 7, 2019

Zlopez commented Jan 8, 2019

codecov-io commented Oct 12, 2018 •

edited

Loading

Zlopez commented Oct 18, 2018 •

edited

Loading

Zlopez Dec 13, 2018 •

edited

Loading