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
feat: new standalone lineage table + associated migration #173
Conversation
An important point to note is that I have assumed that a lineage is inserted in the db when we insert a state and that the associated lineage does not yet exist (otherwise it gets it). |
After further tests I noticed an issue with unique tag on value column of the lineage table which prevent some states to be inserted. EDIT: fixed and ready for review! |
Lineages were introduced in States in Terraboard 0.22.0. Instances that have been running longer than this will have states in their DBs without lineage fields. I think you could insert a Plan in the DB even if there is not yet a State with that lineage, as a new State might come later. Eventually, the interface will have to be redesigned to use lineages for everything. |
db/db.go
Outdated
// Check if the associated lineage is already present in lineages table | ||
// If so, it recovers its ID otherwise it inserts it at the same time as the state | ||
var lineage types.Lineage | ||
if errors.Is(db.First(&lineage, types.Lineage{Value: sf.Lineage}).Error, gorm.ErrRecordNotFound) { |
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.
Maybe it's just me, but I find this line a bit loaded. Can we split into:
if errors.Is(db.First(&lineage, types.Lineage{Value: sf.Lineage}).Error, gorm.ErrRecordNotFound) { | |
err := db.First(&lineage, types.Lineage{Value: sf.Lineage}).Error | |
if errors.Is(err, gorm.ErrRecordNotFound) { |
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.
Arguably, this should never happen this the migration is done when Terraboard is launched. Or am I missing something?
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.
Arguably, this should never happen this the migration is done when Terraboard is launched. Or am I missing something?
Precisely, this condition was introduced by the arrival of the new lineage table. Indeed, when we insert a state there are two possible scenarios:
- The associated lineage was already inserted in the lineages table previously by another state. So, we just recover the lineage id and set the foreign key directly.
- The lineage isn't in the lineages table, so we set the lineage structure in the state one so that gorm takes care of inserting it at the same time as the state
db/db.go
Outdated
TFVersion: sf.TerraformVersion.String(), | ||
Serial: int64(sf.Serial), | ||
LineageID: lineage.ID, | ||
} |
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.
What happens if there's an error but it's not gorm.ErrRecordNotFound
?
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.
On the Gorm.v2 documentation, they only evoke this error for db.First, so I started from the principle that there could be no other ... An interpretation that may be wrong, however, what do you think?
By the way, putting a more generic error handling here will not cost anything and will remove the doubt 🧐
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.
I think it'd be safer to have 3 cases:
err == nil
err
isgorm.ErrRecordNotFound
- else raise an
Unknown error
db/db.go
Outdated
// It will also insert Lineage entry in the db if needed | ||
func (db *Database) UpdateState(st types.State) error { | ||
// Get lineage from old column | ||
if err := db.Raw("SELECT lineage FROM states WHERE path = ?", st.Path).Scan(&st.Lineage.Value).Error; err != nil { |
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.
Isn't there a way do do that using the Go structs?
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 to my knowledge, knowing that it is a request to recover an element which is no longer present in the structure during the migration.
There is however the MigrateColomn
method of Gorm's Migrator interface (https://gorm.io/docs/migration.html) but I'm not sure what it does, I didn't understand the source code and there is no documentation...
* removed lineage field in State Co-authored-by: Raphaël Pinson <github+aem1eeshi1@raphink.net>
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
Lineages are now stored in a separate table and are linked to States / Plans via Foreign Keys constraints.
Thanks to the custom migration introduced, current databases should be compatible and automatically update to the new db model.
The migration is carried out automatically when the database is initialized (Init func in db / db.go) and only if the old "lineage" column is still present in the states table.