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

installer,cli: Add installer web app #1186

Merged
merged 2 commits into from Mar 14, 2015

Conversation

Projects
None yet
2 participants
@jvatic
Copy link
Member

commented Mar 6, 2015

screen shot 2015-03-06 at 5 24 47 pm
screen shot 2015-03-06 at 5 24 58 pm
screen shot 2015-03-06 at 5 25 18 pm
screen shot 2015-03-06 at 5 23 26 pm

@jvatic jvatic force-pushed the installer-gui branch from c622a1f to d761cf3 Mar 6, 2015

@jvatic

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2015

Also, the dashboard doesn't support being passed the login token (I'll open another pull request for that).

usage: flynn install <target> [-n <instances>] [-t <instance-type>] [--aws-access-key-id=<key-id>] [--aws-secret-access-key=<secret>] [--aws-region=<region>] [--vpc-cidr=<cidr>] [--subnet-cidr=<cidr>]
usage: flynn install [<target>] [-n <instances>] [-t <instance-type>] [--aws-access-key-id=<key-id>] [--aws-secret-access-key=<secret>] [--aws-region=<region>] [--vpc-cidr=<cidr>] [--subnet-cidr=<cidr>]
Running flynn install without a target will start a web server for the installer app

This comment has been minimized.

Copy link
@titanous

titanous Mar 6, 2015

Member

Delete the CLI interface for installing. The web UI obsoletes it.

@titanous

This comment has been minimized.

Copy link
Member

commented Mar 6, 2015

Any way to avoid duplicating the cert images and code?

@jvatic jvatic force-pushed the installer-gui branch 7 times, most recently from bb0467a to a1b166b Mar 7, 2015

@jvatic

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2015

@titanous comments addressed

@jvatic

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2015

I don't like my current solution for sharing the cert install view as it requires updating the Gemfile.lock to propagate changes (i.e. always two commits and delayed feedback as to any dashboard breakage). I think copying the files via the Tupfile is best.

@titanous

This comment has been minimized.

Copy link
Member

commented Mar 7, 2015

Symlinks might also work.

@jvatic

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2015

Symlinks don't work on their own as the files they point to become unavailable.

@jvatic jvatic force-pushed the installer-gui branch 2 times, most recently from 768b37d to a859575 Mar 8, 2015

return answer
}
port := args.String["--port"]
return installer.ServeHTTP(port)

This comment has been minimized.

Copy link
@titanous

titanous Mar 8, 2015

Member

This should also run open or the OS equivalent on the URL. Look around for a list of the equivalent on Linux.

@titanous

This comment has been minimized.

Copy link
Member

commented Mar 8, 2015

When I open the same URL in another tab while deploying it appears to be pretty broken.

@titanous

This comment has been minimized.

Copy link
Member

commented Mar 8, 2015

Loading the install URL after a successful install prompts me to install the CA certificate again. It should send me to the dashboard.

@titanous

This comment has been minimized.

Copy link
Member

commented Mar 8, 2015

The HTTP server should bind to 127.0.0.1:0 so that the port is unpredictable and available.

@jvatic jvatic force-pushed the installer-gui branch from a859575 to 22051e4 Mar 9, 2015

@jvatic

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2015

The dashboard will need to setup CORS for localhost in order to properly test the certificate before redirect (the other option is setting flag in localStorage indicating the cert has been installed, but I think having a verification step is better). This will also allow the installer login to the dashboard instead of redirecting with the token in the URL. One possibility is for the installer to set an ENV variable with the localhost address on the dashboard app.

@jvatic jvatic force-pushed the installer-gui branch 2 times, most recently from 284f2cd to 5485576 Mar 9, 2015

@jvatic jvatic force-pushed the installer-gui branch 2 times, most recently from bdc95ef to f3e27f5 Mar 9, 2015

@jvatic

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2015

@titanous ready for another review pass

var httpInstallerPromtsMux sync.Mutex
var httpInstallerStacks = make(map[string]*httpInstaller)
var httpInstallerStackMux sync.Mutex
var awsEnvCreds aws.CredentialsProvider

This comment has been minimized.

Copy link
@titanous

titanous Mar 10, 2015

Member

I prefer using a struct with fields and handler methods over globals.


func promptHandler(w http.ResponseWriter, req *http.Request, params httprouter.Params) {
id := params.ByName("id")
prompt := httpInstallerPrompts[id]

This comment has been minimized.

Copy link
@titanous

titanous Mar 10, 2015

Member

Looks like this needs a mutex lock?

}
if s == nil {
w.WriteHeader(404)
} else {

This comment has been minimized.

Copy link
@titanous

titanous Mar 10, 2015

Member

Return after the 404 instead of using else.

@titanous

This comment has been minimized.

Copy link
Member

commented Mar 10, 2015

What's left on building the assets and putting them into the binary?

@jvatic jvatic force-pushed the installer-gui branch 2 times, most recently from ce1fd92 to dd785f2 Mar 10, 2015

@jvatic

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2015

@titanous comments addressed

Assets are bundled into bindata.go with go generate (requires bundle exec rake compile to be run in the app directory first).

resChan: make(chan *httpPrompt),
api: s.api,
}
prompt.api.InstallerPromptsMux.Lock()

This comment has been minimized.

Copy link
@titanous

titanous Mar 10, 2015

Member

s/Mux/Mtx/

@jvatic jvatic force-pushed the installer-gui branch 7 times, most recently from bae8e79 to 4e4ffab Mar 10, 2015

@jvatic

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2015

@titanous asset compilation via go-bindata is moved into the build process.

@jvatic jvatic force-pushed the installer-gui branch 6 times, most recently from e624abb to 7a978b9 Mar 12, 2015

@jvatic

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2015

@titanous ready for final review

@jvatic jvatic force-pushed the installer-gui branch from 7a978b9 to a0ec5d2 Mar 12, 2015

jvatic added some commits Feb 28, 2015

installer,cli,dashboard: Add installer web app
Makes `flynn install` run web app interface for installer and removes
all other options.

The install cert view is shared between the installer and dashboard.

Signed-off-by: Jesse Stuart <jesse@jessestuart.ca>
dashboard: Don't freeze version string (gem)
Signed-off-by: Jesse Stuart <jesse@jessestuart.ca>

@jvatic jvatic force-pushed the installer-gui branch from a0ec5d2 to 67e39eb Mar 14, 2015

@titanous

This comment has been minimized.

Copy link
Member

commented Mar 14, 2015

LGTM

jvatic added a commit that referenced this pull request Mar 14, 2015

Merge pull request #1186 from flynn/installer-gui
installer,cli: Add installer web app

@jvatic jvatic merged commit 0f4a636 into master Mar 14, 2015

1 check passed

continuous-integration/flynn The Flynn CI build passed
Details

@jvatic jvatic deleted the installer-gui branch Mar 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.