-
Notifications
You must be signed in to change notification settings - Fork 5
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
Simple "server" implementation for catalog. #80
base: master
Are you sure you want to change the base?
Conversation
bf02e7f
to
0ddffae
Compare
Generates the html and serves. It requires a different url-prefix for links so beware of that. We also need to think about how to handle mirrors and download links more effectively.
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 like where this is going :)
fmt.Printf("published HTML for catalog %q to %s\n", pcfg.catalogName, cfg.OutputPath) | ||
|
||
dirHandler := http.Dir(cfg.OutputPath) | ||
fmt.Printf("Serving at http://%s\n", addr) |
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 add some more hints here like "You can now use ca+http://%s/_wares/ as a warehouse URL to fetch content available from this workspace"?
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.
Adding the actual feature behind that should be just a mux, a few lines below here, roughly:
mux := NewServeMux()
mux.Handle("/", http.Dir(cfg.OutputPath))
mux.Handle("/_wares/", http.Dir(workspaceWareDir)
(well, and then get the workspace info down here, so we actually have that path.)
&cli.StringFlag{ | ||
Name: "download-url", | ||
Usage: "URL for warehouse to use for download links", | ||
}, |
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.
Let's get rid of this... and make the server put this workspace's .warpforge/warehouse/
directory online at _wares
and just commit to that.
IMO, the overall behavior of this feature should be: user wants to immediately share; is on their LAN or otherwise not currently doing any other configuration; and just wants a result that works. So we should pick defaults that get stuff done and just go with them.
For this feature in particular, I don't see any reason to leave it configurable at all. We use this in the full static site-gen tool because it lets us account for stories like "my storage bucket and my website are on two totally different domains", etc... but that's never going to be even possible for this kind of "user on a LAN wants results fast" story.
type protoSiteConfig struct { | ||
catalogName string | ||
outputPathOverride string | ||
urlPrefix string | ||
downloadUrl string | ||
} |
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.
Not sure I understand why this struct is needed, which is almost exactly a repeat of the existing SiteConfig struct?
&cli.UintFlag{ | ||
Name: "port", | ||
Aliases: []string{"p"}, | ||
Usage: "Port number for the server address", | ||
Value: 8080, | ||
}, |
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.
Maybe take a whole --addr
flag and default it to ":7070" (with the colon)?
(I'd also just as druther pick a different default port number than 8080. We're not doing a website tool demo (where that number is a norm) so much as we're doing a specific piece of infrastructure here... so I think we're justified in picking a number less likely to collide.)
|
||
dirHandler := http.Dir(cfg.OutputPath) | ||
fmt.Printf("Serving at http://%s\n", addr) | ||
if err := http.ListenAndServe(addr, http.FileServer(dirHandler)); err != 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.
I know I shouldn't pick at a first-pass, but it drives me nuts that the message above gets printed before we actually try to bind :)
I'd like it a lot better if we did listener, _ := net.Listen("tcp", addr)
, then make an srvr := https.Server{http.FileServer(dirHandler)}
, then srvr.Serve(listener)
.
Bonus one is separate error handling for the bind, which is the main thing that can go wrong.
Bonus two is we can also ask listener.(*TCPListener).Addr()
, and then get the real address (e.g. complete with IP address if someone an address spec like ":7070" that's just port) back out, and give the user a URL with that in it. (I think. Haven't tested this.)
&cli.StringFlag{ | ||
Name: "output", | ||
Aliases: []string{"o"}, | ||
Usage: "Output path for HTML generation", | ||
}, |
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.
Might prefer to get rid of this, per other comments about making this as works-by-default as possible. We can just pick a path under {rootWorkspaceControlDir}/tmp/
(or $WARPFORGE_RUNPATH
?)
&cli.StringFlag{ | ||
Name: "url-prefix", | ||
Usage: "URL prefix for links within generated HTML", | ||
Value: "/", | ||
}, |
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.
Let's drop this too, per other comments about making this as works-by-default as possible. We control the whole mux that's processing these URLs and not supporting configuring it, so this option doesn't even make sense in that story.
@@ -49,6 +49,9 @@ Generates HTML output for the root workspace catalog containing information on m | |||
### mirror | |||
Mirror the contents of a catalog to remote warehouses | |||
|
|||
### serve | |||
Generates html and serves |
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.
Propose:
Serve a local http site that contains a navigable copy of the Catalog, and hosts a warehouse at
_wares/
. (Meant for rapid sharing in LAN environments.)
Generates the html and serves. It requires a different url-prefix for links so beware of that. We also need to think about how to handle mirrors and download links more effectively.