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: add plan db scheme #164

Merged
merged 8 commits into from Jun 16, 2021
Merged

feat: add plan db scheme #164

merged 8 commits into from Jun 16, 2021

Conversation

hbollon
Copy link
Member

@hbollon hbollon commented Jun 7, 2021

I used the db scheme available in the TerraVerge repository to which I added a foreign key to the associated bucket (I thought it would be useful if we add support for several simultaneous bucket) and the CreatedAt / UpdatedAt columns

Here is the table that it creates (visualized from pgadmin):
Capture d’écran de 2021-06-07 12-05-45

@coveralls
Copy link

coveralls commented Jun 7, 2021

Coverage Status

Coverage decreased (-0.5%) to 13.093% when pulling e44d389 on hbollon:db-scheme-plan into 1165c06 on camptocamp:v2.

@mcanevet
Copy link
Contributor

mcanevet commented Jun 7, 2021

@hbollon don't we have the lineage in the plan? I guess it would be useful to link it to the states.

Copy link
Member

@cryptobioz cryptobioz left a comment

Choose a reason for hiding this comment

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

Where is the field corresponding to the bucket? Is that the field Source?

types/db.go Outdated Show resolved Hide resolved
types/db.go Outdated Show resolved Hide resolved
@hbollon
Copy link
Member Author

hbollon commented Jun 7, 2021

@hbollon don't we have the lineage in the plan? I guess it would be useful to link it to the states.

No it is not in the model currently but it's a good idea I'll add a field for that

Where is the field corresponding to the bucket? Is that the field Source?

The field representing the associated bucket is VersionID. The Source field on the other hand, if I am referring to TerraVerge, is a trace of the system at the origin of the Terraform plan (like the Gitlab pipeline)

@cryptobioz
Copy link
Member

The field representing the associated bucket is VersionID. The Source field on the other hand, if I am referring to TerraVerge, is a trace of the system at the origin of the Terraform plan (like the Gitlab pipeline)

Okay, that's great! Regarding the bucket field, would it make sense to name it like BucketID or StateID?

@hbollon
Copy link
Member Author

hbollon commented Jun 7, 2021

The field representing the associated bucket is VersionID. The Source field on the other hand, if I am referring to TerraVerge, is a trace of the system at the origin of the Terraform plan (like the Gitlab pipeline)

Okay, that's great! Regarding the bucket field, would it make sense to name it like BucketID or StateID?

I thought the same thing but the structure for the bucket is called Version in the Go code (defined in types/db.go), we would lose readability in the code by renaming this field BucketID while it references a Version no?

@cryptobioz
Copy link
Member

You're totally right, I didn't read the whole file... Then I'm okay with what you did :)

@raphink, could you give us some explanation regarding how you store the S3 bucket information?

@hbollon
Copy link
Member Author

hbollon commented Jun 7, 2021

Another notable thing that I would have wanted you to validate is the use of the go-gorm/datatypes library used to be able to use a jsonb field for store the raw plan output.
That's ok?

@cryptobioz
Copy link
Member

Another notable thing that I would have wanted you to validate is the use of the go-gorm/datatypes library used to be able to use a jsonb field for store the raw plan output.

I guess it's okay for me :)

types/db.go Outdated Show resolved Hide resolved
types/db.go Outdated Show resolved Hide resolved
@hbollon
Copy link
Member Author

hbollon commented Jun 7, 2021

I just thought that the CreatedAt and UpdatedAt fields being managed automatically by Gorm during the insertion, we will not have the real date of creation of the plan. Will it be useful to add a field to store it?

@cryptobioz
Copy link
Member

Well, if everything is automated, you will have the right creation date. We should keep these fields and let Gorm manage them.

@hbollon
Copy link
Member Author

hbollon commented Jun 8, 2021

I think my understanding of the Version structure was wrong, I thought it represented the Bucket but in reality this structure represents the version of a State.
So it seems to me that it is not relevant to have a Belongs To relation on a Plan to a Version right?

I don't understand the comment of this object by the way, "Version is an S3 bucket version", wouldn't it be "a State Version" instead? If I'm not mistaken, with Terraform we don't version Buckets but States files right?

@mcanevet
Copy link
Contributor

mcanevet commented Jun 8, 2021

Indeed, I guess the link between state and plane should be done on lineage, which is, from my understanding, the unique ID that identifies a workspace. lineage+serial identifies a version of the state. lineage+"execution time" should identify a plan.

@hbollon
Copy link
Member Author

hbollon commented Jun 8, 2021

Indeed, I guess the link between state and plane should be done on lineage, which is, from my understanding, the unique ID that identifies a workspace. lineage+serial identifies a version of the state. lineage+"execution time" should identify a plan.

Okay so I removed it. Now this PR should be mergeable if it's all good for you 😄

types/db.go Outdated Show resolved Hide resolved
types/db.go Outdated Show resolved Hide resolved
types/db.go Outdated Show resolved Hide resolved
types/db.go Outdated Show resolved Hide resolved
types/db.go Outdated Show resolved Hide resolved
* added varchar(10) constraint to State TFVersion
* replaced CreatedAt/UpdatedAt
@raphink
Copy link
Contributor

raphink commented Jun 16, 2021

Let's merge this for now so @hbollon can start working on gorm.v2.

@raphink raphink merged commit 2c1cd8f into camptocamp:v2 Jun 16, 2021
@hbollon hbollon deleted the db-scheme-plan branch June 16, 2021 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants