-
Notifications
You must be signed in to change notification settings - Fork 9
(DEPRECATED) Bump decidim from v0.29.2 to v0.30.0.rc1 #141
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
Conversation
@alecslupu I see too many unecessary changes here related to decidim/decidim#13624 Basically there's a mini bug where we're deleting an empty line after the comment with the modification time in the migration files: Apart from that, I think that we actually should be a bit more honest with the changes here. The logic should be:
I'm seeing the result of this diff and it isn't honest, and it might be a pain to debug in the future (like you only would realize that this migration was changed if you check the diff): |
Mind that I had to manually change the `db/schema.rb` file, as the migrations for this application are broken: in 2017 we did a lot of manual migrations that doesn't exist upstream (in decidim/decidim) and that generates a problem locally and in testing where we can't run the `db:migrate` task in a new database, but we need to do the `db:schema:load` or `db:test:prepare` that loads the schema from the schema.rb file instead of running each of the migrations. Some examples of these files are: - db/migrate/20170606113833_add_index_to_accountability_results_on_external_id.decidim_accountability.rb - db/migrate/20170128100053_allow_erased_users.rb - db/migrate/20170405084212_fix_email_uniqueness_index.rb - db/migrate/20170125130555_remove_attachments_table.rb
4bcb618
to
86effd0
Compare
Actually, I would say there is no bug. This change is actually coming from #13690, and the code that actually does that, is taken directly from rails's core. I would say that we can add that extra comment like:
Between the 2 mentioned files, the Rails upgrade from 6.1.7.6 to 7.0.8.4 happened. I would say that we do not need to fix this comment |
Ahhh ook. Then yeah, if it's already there we can leave it out then!
That'd be great! I think some installations can be bitten by this in the future and if you think that the migration wasn't changed when it was actually changed then it can be pain. To be honest this is something that we should have done each time we changed old migrations upstream too. |
Closing this one in favour of #142 |
This PR updates to the last release candidate version, released yesterday
Mind that there are some changes that need to be handled server-side: https://github.com/decidim/decidim/releases/tag/v0.30.0.rc1