Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Support postgres extra options [gcp] #27

Merged
merged 3 commits into from
Nov 6, 2019
Merged

Conversation

tnsetting
Copy link
Contributor

Right now it is enforced to use sslmode when flyte admin is connecting to postgres and password provided. The PR make it possible to connect to postgres when disabling sslmode.

Right now it is enforced to use sslmode when flyte admin is connecting to postgres and password provided.
The PR make it possible to connect to postgres when disabling sslmode.
return fmt.Sprintf("host=%s port=%d dbname=%s user=%s password=%s",
p.config.Host, p.config.Port, p.config.DbName, p.config.User, p.config.Password)
return fmt.Sprintf("host=%s port=%d dbname=%s user=%s password=%s %s",
p.config.Host, p.config.Port, p.config.DbName, p.config.User, p.config.Password, p.config.ExtraOptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

@katrogan will this break our production deployment? We do have options: sslmode=disable in our yaml settings I think.

Also, post IAM role integration, the password empty check is no longer a proxy for sandbox deployment.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah it might be better to only format the password when it's not empty

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused. How does this work today without this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If password is provided, it will discard the option and always use sslmode. But cloudsql proxy does not use sslmode

Copy link
Member

Choose a reason for hiding this comment

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

I mean I couldn't find anywhere in the code reading ExtraOptions. I guess I'm missing something.

@wild-endeavor
Copy link
Contributor

Any chance you could also update the postgres_test.go file please? Thank you.

katrogan
katrogan previously approved these changes Nov 1, 2019
@kumare3 kumare3 merged commit ae314d1 into flyteorg:master Nov 6, 2019
honnix added a commit to honnix/datacatalog that referenced this pull request Nov 12, 2019
chanadian pushed a commit to flyteorg/datacatalog that referenced this pull request Nov 13, 2019
schottra added a commit that referenced this pull request Nov 14, 2019
* origin/master:
  Metadata url and base url as items in the auth context (#33)
  OAuth2 (#8)
  Log links merging logic should take log link names into account (#32)
  Generic type support in workflow compiler (#31)
  Fix invalid filter function error message (#30)
  Support postgres extra options [gcp] (#27)
  Fix no auth provider [gcp] (#28)
pingsutw pushed a commit to pingsutw/flyte-monorepo that referenced this pull request Apr 4, 2023
eapolinario pushed a commit to eapolinario/flyte that referenced this pull request Aug 21, 2023
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* Support postgres extra options [gcp]

Right now it is enforced to use sslmode when flyte admin is connecting to postgres and password provided.
The PR make it possible to connect to postgres when disabling sslmode.

* Add test

* Add test for case with pwd but no extra option
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants