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

add incoming issues to local db, fixes #788 #780

Merged
merged 12 commits into from
Nov 3, 2015
Merged

add incoming issues to local db, fixes #788 #780

merged 12 commits into from
Nov 3, 2015

Conversation

hallvors
Copy link
Contributor

r?@karlcow who seems to have read the code already

@hallvors
Copy link
Contributor Author

Hm. I handled Karl's second comment before the first, and now GitHub shows my commit associated with wrong comment - oh well :)


Base.metadata.create_all(bind=issue_engine)

Base2 = declarative_base()

This comment was marked as abuse.

@miketaylr
Copy link
Member

I wasn't asked for review, but this looks good. Will defer to @karlcow.

@hallvors
Copy link
Contributor Author

(Quite likely we should have some error handling if the content of a report doesn't fit those DB column sizes..)

@hallvors hallvors changed the title add incoming issues to local db, fixes #165 add incoming issues to local db, fixes #788 Oct 22, 2015
@hallvors
Copy link
Contributor Author

Two things to explore:

  • Size limits for various fields
  • If we should track deletion of issues, edits to issue description

@hallvors
Copy link
Contributor Author

I wasn't asked for review

I'm just following the guidelines ;) - "don't ask more than one person for review"

@miketaylr
Copy link
Member

@hallvors heh, no issues with what you've done or the process. I was just adding my 2 cents. 💰

@karlcow
Copy link
Member

karlcow commented Oct 26, 2015

I think we should track deletion.
I would be happy to give r+ if we are not using the DB for now for production stuff. So write only and not read, for the purpose of testing on the rest of the code.

Does that sound reasonable?

@miketaylr
Copy link
Member

I think we should track deletion.

I guess that's probably a new issue, to sync with GitHub API (deletion) changes.

So write only and not read, for the purpose of testing on the rest of the code

+1. Let's let it collect data for a few weeks and come back and inspect how it does. Also if we want deletion, we don't want to rely on anything until that's ready.

@miketaylr
Copy link
Member

I would be happy to give r+ if we are not using the DB for now for production stuff. So write only and not read, for the purpose of testing on the rest of the code.

Does that sound reasonable?

@hallvors want to respond and then fix the merge conflicts? Assuming you agree with Karl I'll be happy to merge at that point.

@hallvors
Copy link
Contributor Author

Rebased and fixed conflicts

karlcow added a commit that referenced this pull request Nov 3, 2015
add incoming issues to local db, fixes #788
@karlcow karlcow merged commit e4e7692 into master Nov 3, 2015
@karlcow
Copy link
Member

karlcow commented Nov 3, 2015

Let's see if we get interesting results.

@miketaylr miketaylr deleted the dbdump branch November 3, 2015 06:11
@hallvors
Copy link
Contributor Author

hallvors commented Nov 3, 2015

Wohoo!! :)

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.

3 participants