-
Notifications
You must be signed in to change notification settings - Fork 8
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
Database Re-Structuring #89
Conversation
c43452c
to
737c200
Compare
3e2d346
to
e4da38f
Compare
cdc360a
to
6c07fa6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Update primary key definitions ============================== Now that we have identified the target primary key definitions we can set about implementing them as a series of Alembic migrations. As part of handling this we have to make the following types of changes to a number of tables: * add a new sequence based column to act as the primary key * update nullable settings for some fields, either because the existing settings are wrong or because they should be nullable, but previously were being used in primary key definitions and as such couldn't be configured as nullable. * change the primary key definition, potentially to an added sequence field. This patch adds a new migration step which implements primary key changes. Custom Data Validation ====================== This patch adds custom validation of the servers tables name field, ensuring that the name field cannot be a null/empty value for an update server. XML Data Import Updates ======================= Fixed incremental update to work with tables whose primary key has been converted to an integer sequence; the search for existing rows will look for an exact match for all data fields rather that just the primary key fields. Misc Changes ============ Removed defunct orm_exporttopostgres script. Updated run_standalone.sh to support overriding env settings so as to avoid the need to edit a committed file to test. Added git ignorance of the top-level .vscode directory. Version bumped to 2.0.4 Closes: SUSE-Enceladus#74
6c07fa6
to
2a35f6e
Compare
@@ -13,4 +13,4 @@ dist/ | |||
AUTHORS | |||
ChangeLog | |||
alembic.ini | |||
|
|||
/.vscode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nitpick - this isn't relevant to the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, and I can move to a separate PR if you want ;-)
export POSTGRES_PASSWORD=MasterSlobs | ||
export POSTGRES_DB=postgres | ||
export POSTGRES_HOST=127.0.0.1 | ||
export POSTGRES_USER=${POSTGRES_USER:-snotty} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the (oddball) default values necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are the values that @guangyee setup initially and we have continued to maintain them.
The only reason I change these lines was to allow the script to pick up the values already configured/exported in the dev-env rather than having to edit the script if it was necessary to change them.
ipv6 = Column(postgresql.INET) | ||
|
||
@validates("name") | ||
def validate_name(self, key, value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment on style - the @validates("name")
specificies that we're validating the name attribute; the actual validation performed is that it's not blank on update servers.
Consider def validate_is_not_blank
... and using the key
in the error handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could re-use that validation for deprecatedon and deletedon on Images, as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With regard to deprecatedon and deletedon, I'm assuming that you want to relate the blankness check to the image?
As for the validate_name() validator, since the definition of validator methods is table specific, and the internal implementation is table specific, in that we are checking the self.type field to see if it has a specific value, I'm not sure that it would be worth the effort to finagle things to share an implementation with the equivalent for the images table, as it may actually make the code, which is pretty minimal/simple right now, overly complex.
As regards the choice of validate_name
as the name for this validator method, well it is accurate, apt and succint given that we are validating the name field, and then internally the implemented check is checking for specific conditions that apply to the name field. I'm not sure that the same applies for validate_is_not_blank
given that the name field can be blank for region servers, which would suggest a name of validate_name_is_not_blank_for_update_servers
which is getting somewhat unwieldy.
I suppose we could implement a helper function that raises ValueError if the value is blank, that validate_name
could call... But for that to be generic enough to be shareable between valdators it would need to either take the table, key, value and an exception message fragment as arguments so that it could construct the required exception message, or take the value and exception message itself as arguments... Which would greatly increase the complexity of what is currently relatively simple code, for not a whole lot of benefit maintainability wise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point on using the key argument in the exception message - I'll fix that now.
Per James' suggestion, use the key argument, which will hold `name` to specify the table field when generating the exception message.
Update primary key definitions
Now that we have identified the target primary key definitions we can
set about implementing them as a series of Alembic migrations.
As part of handling this we have to make the following types of changes
to a number of tables:
the existing settings are wrong or because they should be
nullable, but previously were being used in primary key
definitions and as such couldn't be configured as nullable.
sequence field.
This patch adds a new migration step which implements primary key
changes.
Custom Data Validation
This patch adds custom validation of the servers tables name field,
ensuring that the name field cannot be a null/empty value for an
update server.
XML Data Import Updates
Fixed incremental update to work with tables whose primary key has
been converted to an integer sequence; the search for existing rows
will look for an exact match for all data fields rather that just
the primary key fields.
Misc Changes
Removed defunct orm_exporttopostgres script.
Updated run_standalone.sh to support overriding env settings so as
to avoid the need to edit a committed file to test.
Added git ignorance of the top-level .vscode directory.
Version bumped to 2.0.4
Closes: #74