-
Notifications
You must be signed in to change notification settings - Fork 490
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
Proposal: move buffalo app to cmd #91
Conversation
added olympus app. on top of that |
1530f9b
to
a090b4c
Compare
Ref #87 @michalpristas this fixes #84, right? |
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.
Overall direction looks good to me. I left a few nits and the one note about env var fetching.
cmd/olympus/actions/feed.go
Outdated
func feedHandler(s storage.Storage) func(c buffalo.Context) error { | ||
return func(c buffalo.Context) error { | ||
_, err := getSyncPoint(c) | ||
if 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.
can you combine these two lines together:
if _, err := getSyncPoint(c); err != nil {
...
@@ -0,0 +1,3 @@ | |||
{ | |||
"presets": ["env"] | |||
} |
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 you add a newline at the end of the file?
cmd/proxy/actions/storage.go
Outdated
case "disk": | ||
rootLocation, err := envy.MustGet("ATHENS_DISK_STORAGE_ROOT") | ||
if err != nil { | ||
return nil, fmt.Errorf("missing disk storage root (%s)", err) |
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'd rather not rely on an environment variable this deep in the stack. Could you pass the root location as a param to the function? Same with below
<% } %> | ||
</div> | ||
</div> | ||
<% } %> |
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 you add a newline at EOF?
merging this so we can move ahead without ugly conflicts. |
Just to keep root of the repo clear, executables in cmd (cli, webs) shared code in /pkg
If this is not ok, i will abandon, i don't need to have it there, but it might be ok if we go with one repo for multiple buffalo apps (either olympus or registry)
Fixes: #108