Skip to content
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

Add application design ADR #1

Merged
merged 8 commits into from
Oct 2, 2019
Merged

Conversation

jisantuc
Copy link
Contributor

Overview

This PR adds an ADR explaining the planned application architecture and design goals. It's WIP pending additional information about application requirements.

Notes

rendered

Testing

  • read it
  • decide if you hate it

Copy link
Contributor

@notthatbreezy notthatbreezy left a comment

Choose a reason for hiding this comment

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

A few comments, it's a good summary of our discussion and clarifies a few things too.

docs/architecture/adr-0001-app-setup-and-goals.md Outdated Show resolved Hide resolved
docs/architecture/adr-0001-app-setup-and-goals.md Outdated Show resolved Hide resolved
docs/architecture/adr-0001-app-setup-and-goals.md Outdated Show resolved Hide resolved
Copy link
Contributor

@hectcastro hectcastro left a comment

Choose a reason for hiding this comment

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

Solid start to this ADR/project.

docs/architecture/adr-0001-app-setup-and-goals.md Outdated Show resolved Hide resolved
docs/architecture/adr-0001-app-setup-and-goals.md Outdated Show resolved Hide resolved
docs/architecture/adr-0001-app-setup-and-goals.md Outdated Show resolved Hide resolved
consistency and to avoid the appearance of having made that choice final before
this ADR has been reviewed.
- We'll need to determine a good story about feature flagging and probably also
vertically slice user-facing development. Alternatively, if vertical slicing
Copy link
Contributor

Choose a reason for hiding this comment

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

The feature flagging component of this point makes sense to me, but the vertical slicing one doesn't. If the UI and API are decoupled by design, encoding a vertical slicing workflow to minimize friction between changes that span the frontend and backend seems a little brittle. As an alternative, I'm curious if things like a more robust frontend feature flagging framework, or API versioning were considered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose that's right... I was thinking that even with feature flags, we can't turn features on until the backend work is complete, but it doesn't follow that we need to vertically slice, since we just won't turn the flag on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought briefly about API versioning, but it seemed like a nightmare to me yesterday while I was writing. This morning, it seems... maybe not so bad?

I'm thinking through something like --

class ServiceV1[F[_])(modelRoutes: HttpRoutes[F], aoiRoutes: HttpRoutes[F]) {
  val router = Router(
    "/models" -> modelRoutes,
    "/aois" -> aoiRoutes
  )
}

then later,

// OH NO aoi routes are gone a breaking change
class ServiceV2[F[_])(modelRoutes: HttpRoutes[F]) {
  val router = Router(
    "/models" -> modelRoutes
  )
}

I think it's the case that we could then combine those in a Router as:

...
  // assume companion objects have an apply
  val router = Router(
    "v1" -> ServiceV1(modelRoutesV1, aoiRoutesV1).router,
    "v2" -> ServiceV2(modelRoutesV2).router
  )

where modelRoutesV2 === modelRoutesV1, if nothing has changed. This doesn't really seem that bad to me and obviates the need for figuring out feature flagging logic inside the API as well. @notthatbreezy what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

API documentation with swagger becomes a bit tricky, since we'd have to maintain a lot of copies if we want the root spec document to be standalone... but it's possible we can lean on $ref and try to keep separate parts of the API distinctly documented to keep the API docs DRY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re the versioning strategy above, it at least does work:

diff --git a/app-backend/api/src/main/scala/com/rasterfoundry/granary/Server.scala b/app-backend/api/src/main/scala/com/rasterfoundry/granary/Server.scala
index c55a48b..1723565 100644
--- a/app-backend/api/src/main/scala/com/rasterfoundry/granary/Server.scala
+++ b/app-backend/api/src/main/scala/com/rasterfoundry/granary/Server.scala
@@ -10,15 +10,20 @@ import org.http4s.server.Router
 
 object ApiServer extends IOApp {
 
-  val httpApp: HttpApp[IO] = CORS(
+  val httpApp: HttpRoutes[IO] = CORS(
     Router(
-      "/api/hello"        -> HelloService.routes
-    )).orNotFound
+      "/api/hello" -> HelloService.routes
+    ))
+
+  val versioned: HttpApp[IO] = Router(
+    "/v1" -> httpApp,
+    "/v2" -> httpApp
+  ).orNotFound
 
   def run(args: List[String]): IO[ExitCode] =
     BlazeServerBuilder[IO]
       .bindHttp(8080, "0.0.0.0")
-      .withHttpApp(httpApp)
+      .withHttpApp(versioned)
       .serve
       .compile
       .drain

^^ that compiles

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Hector - probably not good to consider an approach without its own ADR - especially as versioning goes.

I think decoupling the frontend and backend development can be done without too much overhead and reliance on feature flags/coordination -- especially at an early stage for project development. If changes are additive we are usually fine doing the backend work first I think. In the case of breaking changes we will have to be more careful -- and that'll deserve its own ADR at that time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 96d5cc3, and also #2 and #3

model-as-a-service with its own (potentially) asynchronous task runner, we
chose for Granary to own this particular piece of infrastructure complexity.

A second example is in the choice of SNS as the notification service. An
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this suggesting that we are planning to defer all notification subscription overhead to users? e.g., there will be no workflow within the application to massage the topic subscription process for users to get notifications—they will be left to their own devices to sort our that process via the AWS docs, or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

Choose a reason for hiding this comment

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

If we think we can get away with it (mostly thinking from a vendor lock-in perspective—not for us, but for anyone who becomes a user of this project), then that seems reasonable. The Open Data on AWS folks have paved the way for us a bit on it being feasible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that helps, but also since we're not thinking of this as an as-a-service thing, and instead are expecting to deploy into client's own AWS (or other cloud provider if someone will pay us to do it) account, I think we get to make slightly stronger assumptions about the accessibility / security of SNS. I think if this were as-a-service we'd have a real nightmare on our hands trying to manage who's allowed to subscribe to what topics, but the deployment story protects us there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think the SNS topic IDs are kind of random so we get a little bit of security through that.

also have the ability to subscribe to those SNS topics and can subscribe
to notifications.

### Impact of continuous deployment
Copy link
Contributor

Choose a reason for hiding this comment

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

This really isn't an actionable comment, but I wanted to surface that I am onboard with this.

My only request is that we all maintain a high level of awareness about the pros/cons of adopting this methodology within this specific context. CD is consistently put on a high pedestal in our industry (and for good reasons), but I am sensitive to how susceptible it is to cargo culting due to how vague its definition can be, and how its value can fluctuate across contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on board for sure -- we sort of accidentally did CD with the Vision app by opting against a production environment. I'm calling it out specifically here so that in the future if this blows up in our faces we'll know that we did it on purpose and have some reference point for what we thought we needed to make it work and how that failed to keep us safe

Copy link
Contributor

@hectcastro hectcastro left a comment

Choose a reason for hiding this comment

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

Somehow, I can no longer comment on the API versioning subthread. But, all I wanted to add is that I don't think we need to nail down the solution here. I imagine that could be its own ADR in the event that we pursue it. I think just having an idea for how to support multiple versions from the start would be good.

model-as-a-service with its own (potentially) asynchronous task runner, we
chose for Granary to own this particular piece of infrastructure complexity.

A second example is in the choice of SNS as the notification service. An
Copy link
Contributor

Choose a reason for hiding this comment

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

If we think we can get away with it (mostly thinking from a vendor lock-in perspective—not for us, but for anyone who becomes a user of this project), then that seems reasonable. The Open Data on AWS folks have paved the way for us a bit on it being feasible.

@jisantuc jisantuc changed the title [wip] Add application design ADR Add application design ADR Sep 30, 2019
@jisantuc jisantuc merged commit 65e42bf into develop Oct 2, 2019
@jisantuc jisantuc deleted the feature/js/add-app-setup-adr branch October 2, 2019 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants