Conversation
|
/test all |
|
Skipping CI for Draft Pull Request. |
|
/test all |
|
Images are ready for the commit at c58006a. To use with deploy scripts, first |
| "github.com/stackrox/rox/migrator/types" | ||
| pkgMigrations "github.com/stackrox/rox/pkg/migrations" | ||
| pkgSchema "github.com/stackrox/rox/pkg/postgres/schema" | ||
| pkgSchema "github.com/stackrox/rox/migrator/migrations/frozenschema/v73" |
There was a problem hiding this comment.
I think we should call this frozenSchema to make it stand out.
|
a README or something might be useful to explain the reasoning for separating the schema as well as how migrations work going forward. Things like when we lock the migration schema, how we work migrations for the next release. How that impacts the generated code. |
Nice idea. Will do |
md2119
left a comment
md2119
left a comment
There was a problem hiding this comment.
The frozen schema seems to be differ than the ones in pkg/postgres/schema.
| GroupsSchema = func() *walker.Schema { | ||
| schema := walker.Walk(reflect.TypeOf((*storage.Group)(nil)), "groups") | ||
| return schema | ||
| }() |
There was a problem hiding this comment.
The schema seems different. The registration to global table is missing:
GroupsSchema = func() *walker.Schema {
schema := GetSchemaForTable("groups")
if schema != nil {
return schema
}
schema = walker.Walk(reflect.TypeOf((*storage.Group)(nil)), "groups")
RegisterTable(schema, CreateTableGroupsStmt)
return schema
}()
There was a problem hiding this comment.
That is on purpose. The dependency are pre-defined and the tables are not to be reused. The schemas are local to this section of migrations, we do not have global schema in migrator.
There was a problem hiding this comment.
Okay. What I am worried about is that event if the migrator and its schema is frozen, one can still make changes to the schema/proto which actually will go unhandled. I think we should add a check which scan pkg/schema/ dir for any changes. If the changes are desirable, the file is added to a whitelist. WDYT?
There was a problem hiding this comment.
The migration should just have the schema at 3.73. It should not contain future changes to schema. That is what we expect.
md2119
left a comment
md2119
left a comment
There was a problem hiding this comment.
Approving because I do not want to block the work, but I feel a required check that fails the CI would be better for maintenance than dropping schema into new pkg.
3 participants
Description
This PR wont merge before 3.73 branching out.
Freeze 3.73 schema in migrator.
I did not remove the auto generator because I want to keep that in a clean PR dedicated for stopping generators so we can conveniently re-use that PR in case we want to make massive change.
Checklist
[ ] Unit test and regression tests added[ ] Evaluated and added CHANGELOG entry if required[ ] Determined and documented upgrade steps[ ] Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)If any of these don't apply, please comment below.
Testing Performed
TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why
you did not do so. Valid reasons include, for example, "CI is sufficient",
"No testable changes". Feel free to attach JSON snippets, curl commands,
screenshots.
In addition to reviewing your code, reviewers must also review your testing
instructions and make sure they are sufficient.