Open Stack

Anna Taraday <akamyshnikova at mirantis.com> wrote:
> Henry, thanks for taking care of this!
> 
> In my opinion, it is just safe to use raw values in migration, because
> migration is a strict point in time.
> 
> I remember how many patches I send in havana in Neutron for fixing
> synchronization issues. Usage constants everywhere can be good in this case,
> but ModelMigrationSyc did such check of this for us already.
> 
> If we want to have constants everywhere, we should guarantee that they are
> unchanged - have test in neutron-lib which verifies their values. 

Yes, we could have some tests, although a patch changing a constant would
probably also have a change to the test. :) We might need to also have a
prominent "Warning! Do not change this test!" comment for each test.

> 
> 
> On Sat, Oct 15, 2016 at 10:41 PM Henry Gessau <HenryG at gessau.net
> <mailto:HenryG at gessau.net>> wrote:
> 
>     Hi neutrinos,
> 
>     In Neutron many attributes are stored in database fields. The size of these
>     fields therefore determines the maximum length of the attribute values.
> 
>     I would like to get some consistency in place around how we define the
>     constants and where they are used. Here are my thoughts...
> 
> 
>     1. Raw sizes in alembic migrations
> 
>     In the alembic migrations which build the DB schema, we should use the raw
>     number value of the field size.
> 
>     2. FOO_FIELD_SIZE in the sqlalchemy models
> 
>     In the sqlalchemy models, we should use the <field>_FIELD_SIZE constants
>     defined in neutron_lib/db/constants.py
> 
>     3. Everywhere else, use FOO_FIELD_SIZE, or another constant (like FOO_MAX_LEN)
>     based on FOO_FIELD_SIZE.
> 
> 
>     "Why raw numbers in alembic migrations?", you may ask. Well, we have tests
>     that verify that the models match the schema generated by migrations. If both
>     the models and the migrations use the constants then the tests would not
>     detect if a patch changes the constant value.
> 
>     By using raw numbers in migrations, together with our rule of not allowing
>     changes to existing migrations, we allow the tests to detect and fail on any
>     attempt to alter a FIELD_SIZE constant.
> 
>     Let me know if this makes sense or if you think it's a terrible idea.
> 
>     If there are no objections, I intend to submit a patch or patches to:
>      - replace constants with numbers in existing migrations
>      - ensure all models use the appropriate constants
> 
>     Existing code uses FOO_MAX_LEN in a lot of places. In most of these places it
>     would make sense to simply switch to using FOO_FIELD_SIZE. However, some code
>     may be quite far removed from the DB and would look better by sticking to
>     FOO_MAX_LEN. I added item 3. above to allow for that.