-
Notifications
You must be signed in to change notification settings - Fork 1
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 support for customization #56
base: main
Are you sure you want to change the base?
Conversation
fb4ec58
to
182ea9b
Compare
37b5b15
to
b94b487
Compare
3b9031c
to
3a45647
Compare
Signed-off-by: Ales Verbic <[email protected]>
3a45647
to
6422e3a
Compare
@@ -1,12 +1,12 @@ | |||
{ | |||
"AlonzoGenesisFile": "/genesis/preview/alonzo-genesis.json", | |||
"AlonzoGenesisFile": "/opt/cardano/config/preview/alonzo-genesis.json", |
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 it stands now, we are not really using this configs at all (something tells me you wont either). It's kind of legacy for us, from when we used to build our own images, now we use yours
@@ -152,6 +152,7 @@ resource "kubernetes_deployment_v1" "db_sync" { | |||
requests = var.dbsync_resources.requests | |||
} | |||
|
|||
# TODO update to use support custom config not only empty and default config |
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.
I think this makes sense, to change this to
variable args {
type = list(string),
default = []
}
I was checking out the way we use this, and we only set arguments for prime-testnet
dbsync which is different in several ways, so it definitely makes sense as a change
"kind" = "ClusterIssuer" | ||
"name" = var.cluster_issuer | ||
} | ||
"secretName" = local.cert_secret_name |
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.
we should use the var.cert_secret_name
instead if we are going through this route. Also keep in mind that if we are going to do one bouncer per network, the secret should include the network as to avoid having colisions
variable "dns_zone" { | ||
default = "demeter.run" | ||
} | ||
|
||
variable "extension_name" { | ||
default = "dbsync-v3" | ||
} | ||
|
||
variable "instance_role" { | ||
default = "pgbouncer" | ||
} | ||
|
||
variable "network" { | ||
type = string | ||
default = "preview" | ||
} |
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.
Given that the network and the dns_zone are only going to be used to generate the dns, I think we should change this to receive the dns directly, instead of hardcoding a way to template it from the parameters. This goes better with the way we are doing it in txpipe with random domain names
variable "network" { | ||
type = string | ||
default = "preview" | ||
} |
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.
Also just to keep in mind, I think this PR doesn't include the change to have a pgbouncer per network, so I think this module shouldn't receive the network
variable
No description provided.