Skip to content

Conversation

flyingcamilo
Copy link
Contributor

@flyingcamilo flyingcamilo commented Jan 27, 2023

The previous setup adheres to the naming convention, and that's a good thing. Unfortunately, however, there are cases in which you have to break with them.

Unfortunately, that is our case. We use external software and have no influence on their naming.

@FxKu
Copy link
Member

FxKu commented Jan 27, 2023

Would rather like to see databaseNameRegexp being configurable. See #667

@flyingcamilo
Copy link
Contributor Author

@FxKu That sound reasonable. I can do it.

@flyingcamilo flyingcamilo changed the title Add flag to disable database name/schema validation. Add a option to change the regexp for database name/schema validation Jan 27, 2023
@flyingcamilo flyingcamilo force-pushed the dashes branch 2 times, most recently from 990057d to b2ca236 Compare January 27, 2023 15:39
@flyingcamilo
Copy link
Contributor Author

Hey @FxKu added in your suggestion.

@flyingcamilo flyingcamilo force-pushed the dashes branch 6 times, most recently from 51cda17 to 87629d9 Compare January 29, 2023 20:33
The previous setup adheres to the naming convention and that's a good thing. Unfortunately,
however, there are cases in which you have to break with them.

Closes: zalando#667
@flyingcamilo
Copy link
Contributor Author

@FxKu Not sure if the e2e test is failing because of me, but I guess not. Is there anything I can do ?

@FxKu FxKu added this to the 2.0 milestone Apr 19, 2023
@FxKu
Copy link
Member

FxKu commented Apr 20, 2023

@flyingcamilo thanks for adding the config options. What would now happen with the old handling? https://github.com/zalando/postgres-operator/blob/master/pkg/cluster/cluster.go#L44
https://github.com/zalando/postgres-operator/blob/master/pkg/cluster/database.go#L419

I don't see it touched in this PR.

Some more ToDos:

  • add the parameter also in go code
  • copy from opconfig to internal config

if err = c.readValidateDatabaseNameRegexp(c.OpConfig.DatabaseNameRegexp); err != nil {
return err
}

Copy link
Member

Choose a reason for hiding this comment

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

It should rather place the existing validation.

// that feature explicitly
if !(c.databaseAccessDisabled() || c.getNumberOfInstances(&c.Spec) <= 0 || c.Spec.StandbyCluster != nil) {
c.logger.Infof("Create roles")

Copy link
Member

@FxKu FxKu Apr 20, 2023

Choose a reason for hiding this comment

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

please do not introduce unrelated changes - even these are just blank lines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Open Questions

Development

Successfully merging this pull request may close these issues.

2 participants