-
Notifications
You must be signed in to change notification settings - Fork 42
feat(local): add billing platform service #1991
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(local): add billing platform service #1991
Conversation
|
👋 Atrax1, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
skudasov
left a comment
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.
LGTM, but why not to use testcontainers-go directly? It has PostgreSQL, and it'd be the same amount of code without docker-compose for your case @Atrax1
That's fine if you prefer docker compose, but please add a test to the "golden" pipeline and docs.
| } | ||
|
|
||
| if in.UseCache { | ||
| if in.Output != nil { |
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.
you can combine that into if in.UseCache && in.Output != nil
| } | ||
|
|
||
| type PostgresOutput struct { | ||
| } |
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.
Do we you think it makes sense to expose Postgres? What are the use cases for connecting directly to the database?
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.
for migrations
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.
And test querying. CRE can post credit usage to the billing service so it will be useful for us to verify in tests.
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.
CRE can post credit usage to the billing service so it will be useful for us to verify in tests.
seem better though an api than direct db access. consider change the billing service in follow up
| } | ||
|
|
||
| framework.L.Debug().Msgf("Container %s is connected to network %s", billingContainer.ID, networkName) | ||
| } |
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.
this probably could be extracted, defined as a function and used here and in chip ingress
| }, | ||
| } | ||
|
|
||
| framework.L.Info().Msg("Chip Ingress stack start") |
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.
update the log message
| func composeFilePath(rawFilePath string) (string, error) { | ||
| // if it's not a URL, return it as is and assume it's a local file | ||
| if !strings.HasPrefix(rawFilePath, "http") { | ||
| return rawFilePath, nil |
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.
pls modify it to match what we currently do for chip ingress (I modified it last week, i.e we require that local file is prefixed with file://)
| } | ||
|
|
||
| return tempFile.Name(), nil | ||
| } |
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.
actually this should probably also be encapsulated in a function and reused
| return nil | ||
| } | ||
| } | ||
| } |
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.
export these functions and reuse?
| restart: on-failure | ||
| tty: true | ||
| container_name: billing-platform-service | ||
| image: ${BILLING_PLATFORM_SERVICE_IMAGE:-} |
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.
can we have a default image?
| WORKFLOW_OWNERSHIP_PROOF_SERVER_HOST: 0.0.0.0 | ||
| STREAMS_API_URL: ${STREAMS_API_URL} | ||
| STREAMS_API_KEY: ${STREAMS_API_KEY} | ||
| STREAMS_API_SECRET: ${STREAMS_API_SECRET} |
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.
can these values be empty? or should we verify in code whether they are set?
| - "5432:5432" | ||
|
|
||
| migrations: | ||
| image: ${BILLING_PLATFORM_SERVICE_MIGRATION_IMAGE:-} |
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.
how about a default image?
| - dbmate | ||
| - up | ||
| - -- | ||
| - --url=${DATABASE_URL} |
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.
can we have a default?
@skudasov as I see it the whole idea is to be able quickly replace |
e2acbfa to
da7c88e
Compare
9429588 to
c312577
Compare
c312577 to
f795c80
Compare
| } | ||
|
|
||
| const ( | ||
| DEFAULT_STACK_NAME = "billing-platform-service" |
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.
nit/ billing or billing-platform ?
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 var name or the value?
| DEFAULT_BILLING_PLATFORM_SERVICE_BILLING_GRPC_PORT = "2222" | ||
| DEFAULT_BILLING_PLATFORM_SERVICE_CREDIT_GRPC_PORT = "2223" | ||
| DEFAULT_POSTGRES_PORT = "5432" | ||
| DEFAULT_BILLING_PLATFORM_SERVICE_SERVICE_NAME = "billing-platform-service" |
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.
why is this repeated?
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.
Good question. No reason.
| // set development defaults for necessary environment variables and allow them to be overridden by the host process | ||
| envVars := make(map[string]string) | ||
|
|
||
| envVars["MAINNET_WORKFLOW_REGISTRY_CHAIN_SELECTOR"] = strconv.FormatUint(in.ChainSelector, 10) |
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.
add a code comment to New that tells the user about all the env vars.
is there no static file-based 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.
This is how the billing platform service is structured. In an effort to not make any hard references to the billing service repo, I forgo copying a standard .env file and set the relevant env vars manually.
| stack.WaitForService(DEFAULT_BILLING_PLATFORM_SERVICE_SERVICE_NAME, | ||
| wait.ForAll( | ||
| wait.ForLog("GRPC server is live").WithPollInterval(200*time.Millisecond), | ||
| wait.ForListeningPort(DEFAULT_BILLING_PLATFORM_SERVICE_BILLING_GRPC_PORT), |
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.
should this be settable? it will fail if there is a port conflict, right?
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 can do that
| framework.L.Debug().Msgf("Connecting billing-platform-service to %s network", networkName) | ||
| connectCtx, connectCancel := context.WithTimeout(ctx, 30*time.Second) | ||
| defer connectCancel() | ||
| if connectErr := utils.ConnectNetwork(connectCtx, 30*time.Second, cli, billingContainer.ID, networkName, identifier); connectErr != nil { |
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.
this looks odd, that you set the timeout above and repeat the 30s
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.
It does, but it wasn't my doing. The ConnectNetwork utility uses both the connectCtx timeout AND the provided additional timeout internally. I used the same values as the beholder example. ¯_(ツ)_/¯
| tty: true | ||
| command: ["grpc", "billing", "reserve"] | ||
| environment: | ||
| DISABLE_AUTH: true |
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.
oh, is this were all the env vars are set? so that, it is not the case that a test in core would be explicitly setting env vars but rather passing the a ref to the compose file?
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.
This is where the env vars are set in the container. A test could set their own env vars that map to these and would override some of the inputs. I think it's just such a strange case due to the number of env vars in the billing project.
|
|
||
| func ComposeFilePath(rawFilePath string, serviceName string) (string, error) { | ||
| // if it's not a URL, return it as is and assume it's a local file | ||
| if !strings.HasPrefix(rawFilePath, "http") { |
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.
nit/ you can use file:// or http:// and url.Parse to get the protocol
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 was from @Atrax1 but I don't think we need the protocol exactly. Just that it is or isn't http(s).
| return container.Host(ctx) | ||
| } | ||
|
|
||
| func FindMappedPort(ctx context.Context, timeout time.Duration, container *testcontainers.DockerContainer, port nat.Port) (nat.Port, 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.
This would make chaos testing way more complex because when we reboot, we need to find the port again and expose/use this function in chaos tests, minor issue though if you do not plan chaos testing (not recommended!).
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 usually just do static ports in all our services because we know only a single environment is allowed to run both locally and in CI (in CI if you need more you do it with GHA Jobs)
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 am already having issues with mapped ports, but only if I start up this service in the middle of a test. I'm not attempting to run more than one billing service for all tests, but it is still failing when trying to find the port.
| @@ -0,0 +1,93 @@ | |||
| services: | |||
|
|
|||
| billing-platform-service: | |||
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.
So we have a Docker Compose file here and in some other repo, which we can download using HTTP, but why not use only one way?
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 is a good question. I picked this up where @Atrax1 left off. He could comment on why.
This pull request introduces a new Docker Compose-based test component for the billing platform service, including supporting utilities and configuration. The main changes are the addition of a Go package to manage the billing platform service stack, a Docker Compose YAML file to define the service and its dependencies, and utility functions for handling Docker Compose operations and networking. These changes enable automated setup and teardown of the billing platform service in integration tests.
Billing Platform Service Component Implementation:
billing_platform_servicethat provides functions to start, configure, and connect the billing platform service and its dependencies (Postgres, migrations) for testing, including Docker Compose orchestration, container networking, and health checks.Docker Compose Configuration:
docker-compose.ymldefining thebilling-platform-service,postgres, andmigrationsservices, including environment variables, ports, health checks, and dependencies needed for integration testing.Docker Compose Utilities:
utils/docker.gofor downloading Compose files, retrieving container hosts and mapped ports, and robustly connecting containers to Docker networks, to support the new billing platform service component.