mit-dci / opencbdc-tx

A transaction processor for a hypothetical, general-purpose, central bank digital currency
Other
895 stars 200 forks source link

Initial commit of Helm charts and e2e testing #185

Closed kylecrawshaw closed 1 year ago

kylecrawshaw commented 1 year ago

This PR aims to improve validation of changes to opencbdc by setting up a Kubernetes cluster with minikube in Github actions and then install the 2PC architecture. After installation into the minikube cluster there are tests run to check whether or not pods/containers have started and are running. This is a very minimal test that currently does not check anything besides pod/container status. While we are using the Helm chart to run a test this exact configuration can be used to run in any Kubernetes cluster/environment. Documentation should be improved in a future PR to make it clear how to use the Helm chart and customize configuration.

The following has been added:

Signed-off-by: Kyle Crawshaw kyle.crawshaw@gmail.com

kylecrawshaw commented 1 year ago

@HalosGhost @kevinkarwaski This PR should now be ready for review.

HalosGhost commented 1 year ago

@kevinkarwaski as I have much less familiarity with k8s, I'd really appreciate a pretty thorough double-check from you when you have time!

crberube commented 1 year ago

This looks good! Functionally I don't see any issues here, though I have some thoughts and resources on a few areas that I think could be useful to keep in mind.

Error handling

Generally speaking I have found it best to bubble up errors through to the caller, wrapping each error along the way up to the top of the stack. This helps trace errors and keep error handling logic consistent.

E.g.

package main

import (
    "fmt"
    "log"
)

func main() {
  fmt.Println("Doing some logic...")

  err := doSomeLogging()

  // handle err here
  if err != nil {
    log.Fatalf("we tried. then this happened: %v", err)
  }

}

func doSomeLogging(args ...string) error {
    fmt.Println("Logging some stuff...")
    _, err := badNewsBearsFunc()

    if err != nil {
        return fmt.Errorf("error occurred while logging: %s", err)
    }

    fmt.Println("sweet success!")

    return nil
}

I have found this to be a great in-depth article on the subject.

Testing:

As these tests evolve, it looks like they will be a great candidate for table-driven testing. Here is a short article that introduces the concept and here is another that is a little more in-depth. They can really help with the readability and maintanability when you are testing different inputs against expected outputs on the same function.

Package visibility

As i'm sure you have seen whether a name starts with a capital or lowercase letter determines visibility in Go. Digital Ocean has a good article on it. It appears like we could get away with less exposure here as most of this code seems to be all internal to the test package. Along with that, go linting tools will be pretty strict that anything exported, i.e. "public" has a comment (structs, functions, methods, etc.) so that the godoc feature can automatically create docs for it. I've found it be be pretty useful for a minimal amount of extra work.

kylecrawshaw commented 1 year ago

@crberube I've update error handling based on your recommendations and I've also switched to using table testing for the test cases.

@HalosGhost I think we're in good shape here.

HalosGhost commented 1 year ago

@kylecrawshaw I think something may have gone funky; the E2E test takes much longer to complete (actually, it seems to hang for quite a while; I got impatient after about 10 minutes and killed it). I assume this is only happening for me at the moment. Let's plan to chat this week to figure out what may be going wrong.

kylecrawshaw commented 1 year ago

@kylecrawshaw I think something may have gone funky; the E2E test takes much longer to complete (actually, it seems to hang for quite a while; I got impatient after about 10 minutes and killed it). I assume this is only happening for me at the moment. Let's plan to chat this week to figure out what may be going wrong.

@HalosGhost I'll take a look at this today and see what is going on.

HalosGhost commented 1 year ago

@kylecrawshaw This looks great and works lovely locally. Looks like there's a missing DCO and a linting failure, and then I'll be ready to merge!

kylecrawshaw commented 1 year ago

@HalosGhost DCO issue has been resolved and I've fixed the linting error as well. This should be good to now.