Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Modify first h1/p just like npm does #4164

Merged
merged 13 commits into from
Nov 9, 2016
Merged

Conversation

chadwhitacre
Copy link
Contributor

@chadwhitacre chadwhitacre commented Oct 28, 2016

Part of #4148.

@chadwhitacre
Copy link
Contributor Author

I'm going to stop the scheduled job and reprocess already-processed readmes before starting it up again.

@chadwhitacre
Copy link
Contributor Author

Let's get a sense of how long it will take to reprocess readmes ...

@chadwhitacre
Copy link
Contributor Author

Looks like scheduler tasks are not in heroku ps. I'm not sure how to kill it.

@chadwhitacre
Copy link
Contributor Author

Maybe if we remove and then re-add the addon?

@chadwhitacre
Copy link
Contributor Author

Hmm ... heroku logs --tail | grep scheduler isn't turning up anything. Is the process already dead?

@chadwhitacre
Copy link
Contributor Author

watch -n5 "heroku pg:psql -c 'select count(*) from packages where readme_raw is not null'"

/me watches for a minute to see if the number changes ...

@chadwhitacre
Copy link
Contributor Author

It's not changing. I'm going to run a short scheduled job to see if it shows up in heroku ps normally.

@chadwhitacre
Copy link
Contributor Author

screen shot 2016-10-28 at 5 54 18 pm

@chadwhitacre
Copy link
Contributor Author

Yes, scheduler processes show up in heroku ps. 👍

screen shot 2016-10-28 at 5 59 19 pm

@chadwhitacre
Copy link
Contributor Author

Alright, so I'm thinking we should split readme fetching and processing into two separate processes. We can use readme=null to signal a need to process, just as we are using readme_raw=null to signal the need to refetch. We just need to make sure that readme=null shows up in the UI in a sane way. Then we can run both processes as scheduled jobs.

@chadwhitacre
Copy link
Contributor Author

Actually, let's keep readme as the one we read from and display to the user, and add a readme_processed that is set to NULL after fetching.

@chadwhitacre chadwhitacre force-pushed the marky-markdown-package branch from c67d598 to 99b8544 Compare November 8, 2016 22:13
@chadwhitacre
Copy link
Contributor Author

Alright, ready for review. cc: @rohitpaulk @aandis @nobodxbodon @kaguillera et al.

@chadwhitacre
Copy link
Contributor Author

What's up with this marky-markdown failure at Travis? Looks like its installing 9.0.1, which is also what I have locally. The tests pass for me locally.

@chadwhitacre
Copy link
Contributor Author

Drat. Does 9edf5b8 fail in the same way?

@chadwhitacre
Copy link
Contributor Author

No!

@chadwhitacre
Copy link
Contributor Author

Hmm ... now marky-markdown is spitting out markup that I think I saw before, but then switched away from because I thought I saw marky-markdown spitting out different markup ...

@chadwhitacre
Copy link
Contributor Author

Hmm ... might actually be a type discrepancy, Markup vs unicode?

@chadwhitacre
Copy link
Contributor Author

Ah, yeah, at some point this weird inline SVG snuck. O.o Now it's gone again?

@chadwhitacre
Copy link
Contributor Author

Oh! This is over in test_markdown, not in test_npm_sync ...

@chadwhitacre
Copy link
Contributor Author

Here we go, let's try 9ec8a8b ...

@chadwhitacre
Copy link
Contributor Author

Yesssssss. 👍

Gonna clean up a few more things here ...

@chadwhitacre chadwhitacre force-pushed the marky-markdown-package branch from 39d1d07 to 05b5d36 Compare November 9, 2016 00:34
- consolidate markdown tests in one place
- rename render_like_npm for consistency
- add some docstrings/futures/newlines
@chadwhitacre chadwhitacre force-pushed the marky-markdown-package branch from 05b5d36 to 47daf81 Compare November 9, 2016 00:42
At Travis and in production we still want to require YAJL, for
performance, but we don't want to require that in local development.
Note that we still skip the relevant tests if marky-markdown is not
installed, so as to avoid introducing a Node dependency to local
production. Unlike with ijson, which would silently degrade if local
conditions were applied to production, a missing marky-markdown will
hard-fail in production.
With this we have the potential to miss a marky-markdown misconfiguration
at Travis, but not in production.
@chadwhitacre
Copy link
Contributor Author

Okay! I think this is ready to go. What are the next steps once this is merged and deployed? What's our current state and how do we get to a database full of properly-rendered readmes to back #4151?

We're using readme_needs_to_be_updated instead.
@chadwhitacre
Copy link
Contributor Author

gratipay::MAROON=> select count(*) from packages where readme != '';
┌────────┐
│ count  │
├────────┤
│ 133331 │
└────────┘
(1 row)

Heh. :)

I started update packages set readme='' but it takes a while and locks the db(? site was slow to load), so I killed it. I guess it's fine to leave the crap that's in there in there and let it be overwritten by the new processor.

Okay! So I think the thing to do is to:

  • Merge and deploy this.
  • Schedule a fetcher process.
  • Schedule a processing process.

@chadwhitacre
Copy link
Contributor Author

branch.sql sets readme_needs_to_be_processed to true for all records, so even though some readmes are already processed, all should be processed again.

What about refetching? How do we schedule that?

@chadwhitacre
Copy link
Contributor Author

Oh right! upsert is supposed to reset readme_raw to null. Hmm ... maybe another PR? Let's get the first round of readmes in there.

@chadwhitacre chadwhitacre merged commit b7b85fa into master Nov 9, 2016
@chadwhitacre chadwhitacre deleted the marky-markdown-package branch November 9, 2016 02:27
@chadwhitacre
Copy link
Contributor Author

Here we go!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant