themotte / tildes

Other
11 stars 16 forks source link

static/css/site-icons.css missing in development setup script. #24

Closed interstrange closed 4 years ago

interstrange commented 4 years ago

After setting up the vagrant environment, the development setup script (https://docs.tildes.net/instructions/development-setup) encourages users to visit their local site and browse. This is impossible, as the tildes/static/css/site-icons.css file will have not have yet been created, and will cause a missing file exception.

It looks like running the step "Set up a custom user and group (optional)" will, as a side effect, cause this file to be generated. Could be getting created in tildes/tildes/__init__.py and merely importing from the tildes module creates it?

In any case, I guess the solution is either to update the documentation to get users to initialize tildes before attempting to browse their local site, or ensure that site-icons.css (and any other files that could be missing) exist in the repo.

zorbathut commented 4 years ago

Well that's an interesting and timely observation.

Some of what's here is conjecture, because I'm been working on a similar problem, but here's what I've got.

The site-icons.css file is generated by generate_site_icons_css.py, which is supposed to be installed as a cron job. The Salt module that handles this is called consumers, which used to be started from salt/salt/top.sls but was commented out in https://github.com/themotte/tildes/commit/cb185bf113fed21aa2c6699c42f8caed3c5e1b1c .

I actually don't know why it was commented out, but I'm running into similar issues with getting CI working, and part of my incremental fixes have been to undo that. Can you try uncommenting that line out locally and then trying it again?

I think you may need to get salt to reprovision the server, and I have no idea how to do that; alternatively I know you can do that with vagrant destroy && vagrant up.

interstrange commented 4 years ago

I'll test this out - thanks for the pointers. You can assign it to me.

Deimos commented 4 years ago

It's just vagrant provision to re-provision. Salt maintains the state of the server, so this should always be safe to run, and it will only apply whatever changes are new or necessary to fix it to the state it's expecting.

You definitely can't disable the consumers state, a lot of site functionality will just be totally broken without those services running (and apparently the entire site itself?).

The other disabled ones in there are probably fine, if not really correct. Disabling prometheus and nodejs in the dev version means that JS/CSS style checks won't be possible, along with monitoring/metrics. Disabling prod-config will probably mean IPv6 doesn't work on a fresh production server, unless configured manually.

interstrange commented 4 years ago

Sorry @Deimos, by "disable the consumers state" do you mean un-comment the mentioned line in top.sls? I think I'm getting turned around with the multiple negatives.

Are you advising me to not uncomment consumers?

Deimos commented 4 years ago

It should not be commented out.

zorbathut commented 4 years ago

I'm actually kinda curious why this didn't break stuff earlier; my best guess is that everyone who'd tried it since had tried it with a repo that had first been checked out with that uncommented, so it generated a valid css file, and then updated to the commented-out version, which of course would not delete the css file.

Guess we've got some cleanup to do though.

And yeah, consumers should not be commented out, change that; you're welcome to send in a pull request if you want but I'm probably just going to check it in soon under my own power.

interstrange commented 4 years ago

Gotcha - I should add for the moment that in fact the cron job is working, and the file will be generated (making the site usable) after about 5 minutes or so.

So if others are having trouble, it suffices to just be patient for now.

It looks like generate-site-icons-css-cronjob in salt/cronjobs.sls is getting this going. Perhaps it would suffice to just call scripts/generate_site_icons_css.py once as the VM is being set up?

interstrange commented 4 years ago

@zorbathut go ahead!

I'm wondering if un-commenting that consumers file will fix the five minute lag to get the icon file.

Deimos commented 4 years ago

I've actually never noticed this issue, but I tend to do other things after running vagrant up, so it's possible that 5 minutes has just always passed before I try loading the site for the first time. Let me destroy and create a fresh vagrant box and see if it's broken initially, I can just add a salt state that ensures that file exists if it's needed.

Deimos commented 4 years ago

Okay, so the trick is that the file is actually on the host machine and not just inside the Vagrant box. Once it's been created once, it should always be there for any future dev versions set up on the same machine (unless you completely delete the code dir). So that's why I hadn't noticed the problem.

If it doesn't exist, the cronjob will create it, but this would probably still cause issues in a CI setup, since it's all being created fresh each time.

I'll add a fix that either creates the file or makes it so that the app doesn't crash before it exists.

Deimos commented 4 years ago

Alright, this should fix it: https://github.com/spectria/tildes/commit/e85dfa2492bd33a9b019c2e47dc32a281951c151

So then commenting out the consumers state wasn't actually relevant to this, but doing that will definitely break a lot of other things, so you'll want to uncomment that too.

zorbathut commented 4 years ago

Oho, nice! I'll get that merged in shortly, thanks :)

zorbathut commented 4 years ago

(by "shortly" I apparently mean "tomorrow" but it'll happen tomorrow definitely)

zorbathut commented 4 years ago

https://github.com/themotte/tildes/commit/3536dcf75bf4c8a156fec8db5ccc09c7bd12f11f Done, thanks everyone!