Conversation
TODO: Integrate by calling at the right places
hjanott
left a comment
hjanott
left a comment
There was a problem hiding this comment.
I like the general approach and have some suggestions. :)
|
|
||
| #raise MigrationException( | ||
| # f"pre check for migration {module_name} failed." | ||
| #) |
There was a problem hiding this comment.
I would honestly expect the modules function to already do so. No need for the flag.
There was a problem hiding this comment.
Changed it so that a string with all errors is returned from the prerequisites check and thrown if truthy.
| # TODO: removed commented out lines after review | ||
| # MigrationHelper.write_line is hardly usable when only the last | ||
| # line is output. |
There was a problem hiding this comment.
We can change this, but we would need to empty the streams buffer each time reading with truncate(0) and seek(0).
There was a problem hiding this comment.
Changed it so that all lines are read since last time reading. Reads the same block again if nothing new was written.
Still open: - StringIO stream: replace `getvalue` with appropriate `read` / `truncate` - Replace LINK
|
The PR for documentation of the Update to 4.3.0 is now open in main repo https://github.com/OpenSlides/OpenSlides/pull/7091/changes It Includes the file After that the only remaining TODO is:
|
Used for data validations after migration.
|
When calling Originally I was intending to do the pre-check early enough so this wouldn't happen. @hjanott if you find a way for pre-check to fail more gracefully, such that one could just add the missing env vars and call |
|
Please ping @m-schieder if this gets merged. |
|
@peb-adr Because of OpenSlides/openslides-client#5949 (comment) we need to set the timezone for the orga an d meetings. Can we use the ENV variables used here to fill the fields? |
Basically, yes we can. But if we want to have a more readable timezone string in those fields and not just a numeric UTC offset (though it should be an allowed value I think), we should rename If still necessary (not sure at the moment) we can then derive the UTC offset from that. |
Yes we could do that easily. But that must mean that the system was operating in UTC+00. We could also look at the systems time zone and then derive the offset from that. Or as a third option allow to override the calculation by the env variable |
…backend into migration-100-guided
| pass | ||
|
|
||
|
|
||
| class InternalNotImplemented(HTTPException, BaseNotImplemented): |
There was a problem hiding this comment.
Why not use the regular NotImplemented exception?
There was a problem hiding this comment.
This is werkzeug.exceptions.NotImplemented.
I'm strictly following the pattern of how the other Exceptions are created here.
| """ | ||
| Resets the migrations currently in progress and restores the state before the migration. | ||
| """ | ||
| raise CommandNotImplemented("The reset route is not implemented yet.") |
There was a problem hiding this comment.
Another type of "not implemented". Are there good reasons for it?
There was a problem hiding this comment.
See https://github.com/OpenSlides/openslides-backend/pull/3356/changes#r3117186039 This one just holds the error code checked during error handling.
Also I didn't want to name it exactly like the "normal" one, overriding it.
Co-authored-by: peb-adr <adrichter97@gmail.com>
hjanott
left a comment
hjanott
left a comment
There was a problem hiding this comment.
Looks good to me. Thank you for the fix.
peb-adr
force-pushed
the
migration-100-guided
branch
2 times, most recently
from
ff93013 to
f51d62f
Compare
|
Force-pushed back to f51d62f I didn't think the rest through. Probably will open a new PR next week .. |
3 participants
No description provided.