search

gomods / athens

A Go module datastore and proxy
https://docs.gomods.io
MIT License
4.44k stars 502 forks source link

You Might Not Need Buffalo #870

Closed marwan-at-work closed 5 years ago

marwan-at-work commented 6 years ago

There are two reason for making the Athens API Buffalo-less:

  1. The API doesn't need any of the Buffalo features (i.e. frontend asset pipeline)
  2. I, as well as others, have experienced difficulty with setting up and working with the Buffalo ecosystem:

see:

  1. https://gophers.slack.com/archives/C9LRAQN8N/p1541534836483500
  2. https://github.com/gobuffalo/uuid/issues/4
  3. https://github.com/gobuffalo/suite/issues/13
  4. OpenCensus hack: https://github.com/gomods/athens/blob/master/pkg/observ/observ.go#L18
  5. Context key can leak: https://github.com/gomods/athens/blob/master/pkg/log/log_context.go#L10

If/when we introduce a cool UI dashboard, this can be Buffalo'd up, GO-WASM'd up, or whatever the developers of that piece wish to use.

arschles commented 6 years ago

+1 on this!

two thoughts:

Also, #33 could be the start of the cool UI dashboard thinger, for later

marwan-at-work commented 6 years ago

@arschles I think we should use a router that most people are familiar with so that we can onboard contributors more easily, aka gorilla/mux. That being said, totes happy to hear how echo compares and why it might be a good decision for Athens.

penDerGraft commented 6 years ago

"GO-WASM'd up" is an awesome phrase that I would like to use forever 😆

Best issue comment ever.

For additional context (no pun intended) https://gophers.slack.com/archives/C9LRAQN8N/p1541535570496900

I think that the Set and Get methods on the buffalo context are actually intended to make data available in the HTML templates. It works for now with the log entry stuff #809, but it's probably safer to use context.Context for passing data down the stack.

manugupt1 commented 6 years ago

I am going to work on it :D

penDerGraft commented 6 years ago

Just throwing this out there, but would it make sense to keep a checklist of sections of the codebase to track where we've removed it, so as to not remove a major dependency all at once?

We could probably even track this at the individual handler/middleware level though that may be too granular.

@marwan-at-work I think you mentioned still receiving the requests in the handlers and then passing them to the handlers for whatever we're using instead. That way especially with the upcoming releases, we can be non-breaking about it and still merge new changes.

ref: https://gophers.slack.com/archives/C9LRAQN8N/p1541538205546100?thread_ts=1541538205.546100

marwan-at-work commented 6 years ago

@manugupt1 I agree with @penDerGraft -- we should keep this as a proposal and break it down into multiple smaller issues where we break the code endpoint by endpoint, and middleware by middleware. Also, this is post beta, so it'd be nice to wait until everyone has voiced in their ideas on how to approach this refactor :)

manugupt1 commented 6 years ago

@penDerGraft @marwan-at-work @arschles

This preliminary research. I think we might not have to remove buffalo. To me, the issue

  1. is a big refactor

  2. is something that we no longer require as we do not have an rdbms support yet.

  3. is fixed and less likely to occur again

  4. and 5. are context related and this is what has been my main motivation to remove buffalo. What if we start by having our context over buffalo.Context and use it to do things that we wan't it to do. If we can solve this problem with our own context, that will be sweet!

@Lyoness has done this earlier, but by composing over context.Context. What do you think?

manugupt1 commented 6 years ago

Also, here is the relevant documentation: https://gobuffalo.io/en/docs/context

penDerGraft commented 6 years ago

@manugupt1 I don't really have strong opinions either way. However, my understanding is that point 1 was referring to general difficulties that new contributors faced with on-boarding. This was actually true for me as well since I started around the time of the force-push/checksum issue with buffalo and couldn't get my local env to work right.

I will say that I did like the idea of separate repos for backend proxy functionality and any UI/dashboard features that might be added in the future.

manugupt1 commented 6 years ago

@penDerGraft Yep! The proxy functions as an API right now and it might require some clean up, but it is almost there. For example look at this change https://github.com/gomods/athens/pull/898

I think the onboarding issue can be fixed through better documentation overall. We have multiple workflows even amongst us. The documented way needs to be tested or reported. We have fixed it now I believe. I can do a clean installation and double check later though.

The context.Context key is the weird part that we should fix though which was also proposed earlier but in a different way. https://github.com/gomods/athens/issues/348

manugupt1 commented 6 years ago

Okay, I do have a proposal finally. How about we have our own context which has a buffalo.Context but accounts for our own logging and tracing capabilities?

For every interface layer, we just use our own context For eg: https://github.com/gomods/athens/blob/master/pkg/storage/lister.go#L9

An example implementation might look like this. https://play.golang.org/p/qxJZzFqjF8Q

We can also use the function that we have to say, trace and log together.

This is still a big refactor but does not remove buffalo as much.

marwan-at-work commented 6 years ago

@manugupt1 we should keep using the real context.Context interface, because we can in the future face the same problems that we've been facing with buffalo.Context which is incompatibility with other libraries.

Also, this means updating almost every function in the code base and going down a whole new wormhole

That being said, having our Context type means we're in control of its compatibility. But it also means that we'll keep fighting other libraries with type-switching.

manugupt1 commented 6 years ago

@marwan-at-work Yep! A) We should only do this when function signatures are changed and nothing else in the first pass which would be essentially s/context.Context/athensContext/. That is just the number of lines of code that will be changed but not the functionality imo. B) Regarding incompaitibiliy, I can't really speak of that, I think libraries should not force context.Context on everyone. Instead they should take any interface and work with it and not lose any data that the passed context has to it. C) Right on, I am not of applying buffalo.Context simply because of the reason that we were not in control of the compatibility, this time we will be. D) If we end up wanting to remove buffalo, we will still have to implement our own context.Context interface with all the request headers and everything stored in it.

Thoughts!

penDerGraft commented 6 years ago

FWIW I just opened the above PR to give some substance to the idea of removing buffalo incrementally. It's just an example, but maybe seeing how a refactor might look will help in figuring out what we want to do.

arschles commented 6 years ago

FWIW, in case people didn't know:

The API doesn't need any of the Buffalo features (i.e. frontend asset pipeline)

Buffalo has an "API only" mode that you can get by running buffalo new --api thing. I don't have an exhaustive list of differences between frontend and api only mode, but it looks like at least it doesn't have webpack.config.js, assets/ and the package.json files.

I, as well as others, have experienced difficulty with setting up and working with the Buffalo ecosystem:

Without the frontend pipeline, buffalo dev would end up being go run with code reloading. Not sure if that helps with setting up / working with (maybe there are other problems around modules support?)

komuw commented 6 years ago

I would also be in favor of removing Buffalo, for the various reasons that have already been listed;

I do not have strong opinions on what we should replace Buffalo with, so long as it is something that has sane defaults(especially security-wise).

scriptonist commented 5 years ago

I don't know if I'm qualified to make an opinion about this, but I have experienced some onboarding issues with athens. I had to look up buffalo first get some background and jump back to athens. I've contributed some code to athens, from my limited knowledge of the codebase I used to ask my self, Why buffalo is used 🤔 Why didn't they go with something light 🤔 and always concluded that it may be because of some design decision just out of the scope of my knowledge about the project.