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

Redesign shorturls. #253

Closed
wants to merge 1 commit into from
Closed

Redesign shorturls. #253

wants to merge 1 commit into from

Conversation

kkuehlz
Copy link
Member

@kkuehlz kkuehlz commented Nov 30, 2017

Now, make non-permanent shorturls on supernova in ~staff/public_html/short_urls in the .htaccess file.
They can be accessed via ocf.io/s/. Also, file perms are set up so all staff can make shorts.

Talked to baisang and abizer and they specified which shorturls were to be (re)moved.

Note: death is currently on my environment (up to date with master) so you guys can easily test this out.

Now, make non-permanent shorturls on supernova in ~staff/public_html/short_urls in the .htaccess file.
They can be accessed via ocf.io/s/<slug>. Also, file perms are set up so all staff can make shorts.

Talked to baisang and abizer and they specified which shorturls were to be (re)moved.
@@ -74,21 +71,17 @@
{rewrite_rule => '^/printing$ https://www.ocf.berkeley.edu/announcements/2016-02-09/printing [R]'},
{rewrite_rule => '^/rt$ https://rt.ocf.berkeley.edu/ [R]'},
{rewrite_rule => '^/rt/([0-9]+)$ https://rt.ocf.berkeley.edu/Ticket/Display.html?id=$1 [R]'},
{rewrite_rule => '^/s(/[^/]*)$ https://www.ocf.berkeley.edu/~staff/short_urls$1 [R]'},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps make ocf.io/s and ocf.io/s/ redirect to the s-link list?

@dkess
Copy link
Member

dkess commented Nov 30, 2017

I think this is fine as a temporary fix, but I think long-term we should be working towards a database-backed shortlurl store.

@chriskuehl
Copy link
Member

what's the benefit of this besides not having to wait for puppet to run?

@abizer
Copy link
Member

abizer commented Nov 30, 2017

I actually feel like this is fine as a longer-term solution. It solves the immediate problem of (a) there not being a decent place to stash temporary shorturls (b) them polluting Puppet and (c) interfering with account creation. Making it database-backed adds more code and complexity than this problem warrants, IMHO. Plus it's much more self-documenting than something backed by a DB table.

@chriskuehl
Copy link
Member

chriskuehl commented Nov 30, 2017

there not being a decent place to stash temporary shorturls

puppet

them polluting Puppet

what does polluting mean? it's code in a git repo.

interfering with account creation

puppet solves this too if you're willing to use /s/...

@chriskuehl
Copy link
Member

i guess i should clarify: i don't really think puppet is a good place to put these, but having them stored in a structured text file on nfs without version control or tests seems like a particularly bad regression. aren't we just inheriting all the problems we hate about the staff_hours.yaml file?

@abizer abizer closed this Nov 30, 2017
@abizer abizer reopened this Nov 30, 2017
@matthew-mcallister
Copy link
Member

I previously avowed that short URLs would be better off in ocflib, which I argued addresses both @chriskuehl and @abizer's present concerns and saves on having to duplicate every rule in RESERVED_USERNAMES.

@abizer
Copy link
Member

abizer commented Nov 30, 2017

I mean, there's much more overhead to putting things into puppet than just dropping them into a text file.
We can also write a tiny bash script like shorten <url> <slug> that should be fairly idiotproof. Also since these shorturls shouldn't be distributed outside the OCF, if they're broken for a bit it's not really a critical problem even if someone messes up, unlike staff_hours.yaml.

@matthew-mcallister
Copy link
Member

matthew-mcallister commented Nov 30, 2017

@abizer I feel like there are better solutions to dynamic URL shortening than this.

If you only want to create ephemeral, OCF-internal links, then you can either use an existing, third-party link shortening service, or you can just use the .htaccess in your own public_html dir. I see the benefits/drawbacks of either of those as roughly equivalent to this solution.

If you really want a canonical, OCF-flavored link shortener, then I think a structured solution backed by a database and API would be an actual improvement. I still contest that such a system should use /s/ though so it can't wantonly interfere with account creation intentionally or by accident.

@chriskuehl
Copy link
Member

Puppet is literally just adding one line to https://github.com/ocf/puppet/edit/master/modules/ocf_www/manifests/site/shorturl.pp and waiting a few minutes -- you can even do it from your web browser. I guess I don't get why people think this is so hard.

Moving even more stuff to non-versioned non-tested easy-to-break files on nfs is going the wrong direction imo. In ocflib or in a database would both be preferable because it separates the logic (RewriteRule syntax junk) from the data, which this does not do.

I still think a database is probably the best option, as you can then e.g. add support for adding shorturls to the ircbot, to a cli, to a staff web interface, whatever. Account creation could check this database to avoid collisions.

@jvperrin
Copy link
Member

The account creation conflict was the main reason why putting it in a database would be nice (there's an RT ticket about this shorturl redesign already), and then we can also record who added each link, when they were added, stuff like that that is tough to get information about when it's a file. I think doing something similar with staff hours eventually would also make sense, because of their current untracked nature and how many people edit the file and mess up on the formatting. Putting more things in version control seems like a good move to me, and if not version control then a database where you can see who added it and when (then it's close to version control anyway)

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.

7 participants