Enforce unique contraints on object names/labels in the database. #971
Conversation
|
Some help would be much appreciated. I am suspecting that because this is called many time: Line 612 in 45f218d so the node name is not unique in the CI database? |
hil/model.py
Outdated
| @@ -58,7 +58,7 @@ class Nic(db.Model): | |||
| """a nic belonging to a Node""" | |||
|
|
|||
| id = db.Column(BigIntegerType, primary_key=True) | |||
| label = db.Column(db.String, nullable=False) | |||
| label = db.Column(db.String, unique=True, nullable=False) | |||
There was a problem hiding this comment.
This is not actually supposed to be unique; it's scoped to the node, but two nodes may have nics with the same name.
There was a problem hiding this comment.
oops, I should've put it in the class Node
Pull Request Test Coverage Report for Build 1693
💛 - Coveralls |
Pull Request Test Coverage Report for Build 1729
💛 - Coveralls |
|
Could you also write a migration script? Other than that, LGTM. |
|
project names, switch names, network names are unique too. |
| # revision identifiers, used by Alembic. | ||
| revision = '264ddaebdfcc' | ||
| down_revision = '89ff8a6d72b2' | ||
| branch_labels = ('hil') |
There was a problem hiding this comment.
You'll want a trailing comma here; this is supposed to be a tuple.
tests/unit/migrations/flask.sql
Outdated
| @@ -287,7 +287,7 @@ ALTER SEQUENCE nic_id_seq OWNED BY nic.id; | |||
|
|
|||
| CREATE TABLE node ( | |||
| id integer NOT NULL, | |||
| label character varying NOT NULL, | |||
| label character varying NOT NULL UNIQUE, | |||
There was a problem hiding this comment.
Naved and Jeremy suggested that this is not the place that does the migration tests. And the question now is how we can increase the test coverage? Or, where do we do the migration tests?
There was a problem hiding this comment.
We shouldn't ever be modifying this file; the whole point of the tests is to verify that we can cleanly upgrade from an old database schema; this is a snapshot of a database we should be able to upgrade from. What you've done here is manually modify the old schema.
The actual tests are in hil/migrations.py.
The missing coverage in upgrade is incorrect; we're definitely running that in the migration tests (and it seems to be erroneously missing from all of the other scripts as well). downgrade() is really untested, and we basically haven't done testing with downgrading databases at all.
My inclination is to:
- Figure out why the upgrade function doesn't look like it's covered
- Open a separate issue about the fact that we're not testing downgrade() anywhere. Honestly, I'm not sure it's worth the effort to properly support database downgrades, and we've only been adding the function at all because alembic expects it. But we should discuss the issue.
I'm fine opening separate issues for both of these and just overriding the coveralls warning for now.
I agree. |
|
I'm happy. If everyone's in agreement re: coveralls we can just merge. |
|
Cool, merging then. |
Fix: #946
Potentially, we should think about whether there are any other constraints.
And this might break lots of tests. Consider this is an on-going implementation