-
Notifications
You must be signed in to change notification settings - Fork 0
Add script and route to login to Heroku #100
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
Conversation
c5e1667 to
55a1e43
Compare
|
@jelaniwoods Excellent work. Notes as I am testing:
|
🤔 I'm not able to reproduce this but I imagine that maybe the latest version of this branch was not installed. The changes should handle that with a proper message:
Correct, if I can also try doing what we talked about earlier and detecting if I also have a snapshot here of it working: https://gitpod.io/#snapshot/eca1efee-a409-41e5-8e68-de16db85da59 |
This seems like a good idea; also I did run |
Hmm, I just did a |
I can add it to the install command. I'm not entirely sure if I could that part reversible though. |
|
@jelaniwoods Okay when testing on the snapshot:
|
I'm not sure why that would happen, but it did happen to me today when I was trying to reproduce the behavior. I |
|
I might tweak the message a bit; seems to contain a bit of redundancy, no? I tried signing in with wrong password at Heroku and they say: I guess they don't want to leak whether it's email or password that's wrong, i.e. whether there's an account with that email or not. Maybe we could say something with more humanity, e.g. |
|
I don't think the message is redundant since login could fail for a reason besides entering incorrect credentials. When testing the email/password could be blank or the script could timeout. Both of these cases are handled. The message comes straight from |
Oh I see. Yes, I agree the message could be clarified and I like your suggestion. |
|
@jelaniwoods Since I imagine that the Heroku-related functionality may grow beyond only logging in (is that right?), what do you think about tabs across the top of the page so that we're not just constrained to the branches area? It feels like, conceptually, that's what the options are; tabbing between an interface to Eventually I can imagine:
|
|
@jelaniwoods Looking great. What do you think about filling the entire width with the tabs? They feel a little small to me at present. |
|
When I tried to https://gitpod.io#snapshot/68d150dd-2f4b-416a-a054-dd48915de313 |
@raghubetina looks like this is an issue that happens when the installer is run more than once. It adds an extra Not sure why yet, but I guess not all of the methods check if the file needs to be modified. |
Oh this is nice! I didn't really understand how this worked from the documentation example since it's in a smaller width. |
|
Nice! LGTM now |
1058e43 to
2aae34c
Compare
Currently when the installer is run multiple times, additional ends or other Ruby is injected into config.ru which causes app breaking errors. This commit updates the installer to conditionally modify config.ru when it doesn't contain the required Ruby for web_git and allows for the installer to be run multiple times without issue.









Related to #45
Problem
One of the big reasons we use
web_gitis to avoid teaching CLI stuff. Heroku CLI is much simpler to use than git, but since they are very much connected there should also be some web interface for the Heroku commands that are common.Solution
This branch adds Heroku "login" functionality.

This also adds the dependency on
expect, which seems to be the only reliable way to auto-login to Heroku CLI.Concerns
Mentioned also in #45 , but I'm not sure what the best place is to add this in the UI. There are already a lot of forms going on so I opted for a collapse to try and not clutter things up. I know eventually we've talked about a redesign, but I think this is more important than that.Bootstrap tabs are an improvement over a collapse and allow space for further Heroku features in the future.Setup
Gemfileexpectif it isn't already installed)rails s/gitReferences