-
Notifications
You must be signed in to change notification settings - Fork 11
feat: support Airbyte DB with Postgres 17 #162
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
Conversation
So it can be access by other packages.
This will be used to patch the manifest lists.
e2c4595
to
6e6bd28
Compare
6e6bd28
to
ff31206
Compare
} | ||
|
||
if opts.PatchPsql17 { | ||
vals = append(vals, "postgresql.image.tag=1.7.0-17") |
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.
As we reference this specific version of postgres more than once, can we pull it out to a shared location?
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.
Added a couple of fixup commits for this.
internal/cmd/local/local/cmd.go
Outdated
// PatchPsql17 checks if PostgreSQL data needs patching by examining the | ||
// local PostgreSQL data directory. It returns true if the directory doesn't | ||
// exist or contains PostgreSQL version 17. Otherwise it returns false. | ||
func PatchPsql17() (bool, error) { |
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.
Could we rename this to RunningPsql17? Calling it Patch makes me think this method will patch psql to that version.
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 about EnablePostgres17
?
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.
That works too
}) | ||
|
||
pgVersion, err := pgData.Version() | ||
if err != nil && !errors.Is(err, fs.ErrNotExist) { |
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.
errors.Is
will handle the nil case for you, you don't need to check it explicitly
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.
The reason why I can't do that is because I am using !
. It would also return error when err is nil. See - https://go.dev/play/p/CAN89EgXbav
@colesnodgrass I've added a couple of fixups. Let me know what you think about my other two comments. |
No description provided.