-
Notifications
You must be signed in to change notification settings - Fork 28
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
Store(local dir) and Serve(http server) FBC #135
Changes from all commits
5d7eea7
8afa23a
9a2a909
e6b10ff
9d60f83
4e304f9
d9069e9
50842c6
2d64104
e59cdff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,56 @@ | ||||||||
package catalogserver | ||||||||
|
||||||||
import ( | ||||||||
"context" | ||||||||
"net/http" | ||||||||
"os" | ||||||||
"time" | ||||||||
|
||||||||
"golang.org/x/sync/errgroup" | ||||||||
) | ||||||||
|
||||||||
// Instance is a manager.Runnable catalog server instance, | ||||||||
// that serves the FBC content of the extension catalogs | ||||||||
// added to the cluster | ||||||||
type Instance struct { | ||||||||
Dir string | ||||||||
Addr string | ||||||||
ShutdownTimeout time.Duration | ||||||||
} | ||||||||
|
||||||||
func (s Instance) Start(ctx context.Context) error { | ||||||||
server := &http.Server{ | ||||||||
Handler: ServerHandler(s.Dir), | ||||||||
Addr: s.Addr, | ||||||||
ReadHeaderTimeout: 3 * time.Second, | ||||||||
} | ||||||||
eg, ctx := errgroup.WithContext(ctx) | ||||||||
anik120 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
// run a server in a go routine | ||||||||
// server.ListenAndServer() returns under two circumstances | ||||||||
// 1. If there was an error starting the server | ||||||||
// 2. If the server was shut down (ErrServerClosed) | ||||||||
// i.e both are non-nil errors | ||||||||
eg.Go(func() error { return server.ListenAndServe() }) | ||||||||
// waiting for one of two things | ||||||||
anik120 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
// 1. a error is returned from the go routine | ||||||||
// 2. the runnable's context is cancelled | ||||||||
if err := eg.Wait(); err != nil && ctx.Err() == nil { | ||||||||
// we only get here if we're in case 1 (both case 1s) | ||||||||
return err | ||||||||
} | ||||||||
// if the ShutdownTimeout is zero, wait forever to shutdown | ||||||||
// otherwise force shut down when timeout expires | ||||||||
sc := context.Background() | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How bout |
||||||||
if s.ShutdownTimeout > 0 { | ||||||||
var scc context.CancelFunc | ||||||||
sc, scc = context.WithTimeout(context.Background(), s.ShutdownTimeout) | ||||||||
defer scc() | ||||||||
} | ||||||||
// if the runnable's context was cancelled, shut down the server | ||||||||
return server.Shutdown(sc) | ||||||||
} | ||||||||
|
||||||||
func ServerHandler(dir string) http.Handler { | ||||||||
fs := http.FileServer(http.FS(os.DirFS(dir))) | ||||||||
return fs | ||||||||
Comment on lines
+54
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Can reduce this to a one liner:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would also expect The URL paths will come in with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the benefit of making the path /catalogs/catalogName/all.json? Why not just keep it simple to /catalogName.json? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original intention behind |
||||||||
} |
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 want the http server disabled by default? I figure we probably want all of them enabled so we can start the deprecation/removal process after a new release with these changes.
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'm still thinking this should probably be set to true by default so that all of the serving methods are available by default in the next release and we can remove the
Package
,BundleMetadata
, andCatalogMetadata
in the next+1 release. That being said that can be done in a follow up if we want and shouldn't block this PRThere 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 don't think we even have to wait for the next +1 release. We should be fine just removing those, and switching this to true. But yea let's leave that for a follow up