Skip to content
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

Merged
merged 5 commits into from Jun 22, 2021

Conversation

hbollon
Copy link
Member

@hbollon hbollon commented Jun 18, 2021

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.

@mcanevet mcanevet requested a review from raphink June 18, 2021 07:20
@coveralls
Copy link

coveralls commented Jun 18, 2021

Coverage Status

Coverage decreased (-0.3%) to 12.234% when pulling c4d72ef on hbollon:lineage into a8e6bdd on camptocamp:master.

@hbollon
Copy link
Member Author

hbollon commented Jun 18, 2021

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).
On the other hand, if a plan is submit (the endpoint is not yet available) and the associated lineage is missing from the database then it will not be accepted (since this will mean that no state associated with this plan is present) .
Is that good?

@hbollon hbollon marked this pull request as draft June 18, 2021 09:00
@hbollon
Copy link
Member Author

hbollon commented Jun 18, 2021

After further tests I noticed an issue with unique tag on value column of the lineage table which prevent some states to be inserted.
I fix that and re-open this PR

EDIT: fixed and ready for review!

@hbollon hbollon marked this pull request as ready for review June 18, 2021 09:26
@raphink
Copy link
Contributor

raphink commented Jun 18, 2021

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) {
Copy link
Contributor

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:

Suggested change
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) {

Copy link
Contributor

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?

Copy link
Member Author

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,
}
Copy link
Contributor

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?

Copy link
Member Author

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 🧐

Copy link
Contributor

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 is gorm.ErrRecordNotFound
  • else raise an Unknown error

db/db.go Show resolved Hide resolved
types/db.go Show resolved Hide resolved
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 {
Copy link
Contributor

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?

Copy link
Member Author

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>
db/db.go Outdated Show resolved Hide resolved
db/db.go Show resolved Hide resolved
db/db.go Outdated Show resolved Hide resolved
Copy link
Contributor

@raphink raphink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@raphink raphink merged commit aa7d455 into camptocamp:master Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants