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

Ignore the users.nearby column #2432

Merged
merged 1 commit into from
Nov 14, 2019

Conversation

gravitystorm
Copy link
Collaborator

This is the first step of removing the column, see #2417. It needs to be deployed before a migration to remove it, since the columns are cached in ActiveRecord and things break if objects exist in memory
that expect the database column to be there.

Thanks to strong_migrations for pointing this out when I tried creating the migration first!

This is the first step of removing the column, see openstreetmap#2417. It needs to be
deployed before a migration to remove it, since the columns are
cached in ActiveRecord and things break if objects exist in memory
that expect the column to be there.
@tomhughes
Copy link
Member

Well if you really want to pander to strong_migrations extremist nonsense...

@tomhughes tomhughes merged commit dd294f8 into openstreetmap:master Nov 14, 2019
@gravitystorm
Copy link
Collaborator Author

Well if you really want to pander to strong_migrations extremist nonsense...

I'm surprised by your attitude here @tomhughes, it's not very helpful.

Without doing this, if we just ran the migration to drop the column, then anyone in the middle of doing anything with a User object (i.e. already read from the database and then writing to the database) can receive an error. Creating a user, updating their preferences, etc. So you run the migration, and people see errors. I want to avoid that.

I don't consider this to be "pandering to ... extremist nonsense" and it's a) a pretty trivial solution and b) not something that I'm burdening you with.

If I haven't yet convinced you that strong_migrations is useful, then fine, you don't have to agree. But I find it useful, and I like doing migrations properly, so I'll continue to use it.

@tomhughes
Copy link
Member

Yes it might cause a breakage for the couple of minutes between me running the migration and then doing a coordinated deployment on the web servers.

I just think that this, like many of strong_migrations suggestions, creates a lot of work and litters the source with residual noise, for very minimal gains. I believe I made my views on this known when we originally added it ;-)

@gravitystorm
Copy link
Collaborator Author

I just think that this ... creates a lot of work

Creates a lot of work? The two options are:

a) run a migration, which breaks things in production, or
b) apply this one-line patch. Later, run exactly the same migration as in a). Finally, remove this one-line from the source.

Seems like an easy choice to me.

@tomhughes
Copy link
Member

I was assuming you would leave it in. Yes I know that doesn't actually help but it's easy to think it does unless you think it through carefully ;-)

gravitystorm added a commit to gravitystorm/openstreetmap-website that referenced this pull request Nov 20, 2019
@gravitystorm gravitystorm deleted the remove_nearby branch December 18, 2019 15:05
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.

2 participants