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.

Sign up for GitHub

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

Jump to bottom

bug(cmd): add more advanced handling of app urls #110

Merged
merged 1 commit into from
Jul 25, 2016
Merged

bug(cmd): add more advanced handling of app urls #110

merged 1 commit into from
Jul 25, 2016

Conversation

Joshua-Anderson
Copy link
Contributor

Previously, we took a very naive approach to determining the app url. We did appIDdeis.cluster.com

This approach has a problem however, because you can assign subdomains to an app that mask another app:

$ deis domains:add a -a t
$ deis apps:create a --no-remote
$ deis apps:open -a a # opens a.cluster.com, which actually points to t

Now, apps:open and apps:info use the app's first domain, usually the one that is assigned by default. If the app doesn't have any domains, like a in the example does, it'll print that the app doesn't have any domains.

This also allows me to remove api.Apps.URL from the SDK laster, removing the naive behavior in a way that doesn't break anything.

@Joshua-Anderson Joshua-Anderson added this to the v2.2 milestone Jul 1, 2016
@Joshua-Anderson Joshua-Anderson self-assigned this Jul 1, 2016
@deis-bot
Copy link

deis-bot commented Jul 1, 2016

@mboersma, @krancour and @ngauthier are potential reviewers of this pull request based on my analysis of git blame information. Thanks @Joshua-Anderson!

@bacongobbler
Copy link
Member

bacongobbler commented Jul 18, 2016 •
edited
Loading

I'm not sure I like this change. This makes it so -a accepts a domain as well as app IDs rather than strictly app IDs. That makes it inconsistent and could cause confusion among the other commands that only accept app IDs for -a. Why don't we just do a check that app a exists and return a 404 if it doesn't exist instead?

@Joshua-Anderson
Copy link
Contributor Author

@bacongobbler This doesn't affect the behavior of -a at all... this behavior is for looking up the URL for an app when using app:open and app:info. There were a few edge cases where what the previously the assumed app url could actually point to another app. Now, apps:open and apps:info look up the app domain from the controller rather than try to guess it.

@bacongobbler
Copy link
Member

Ah sorry, now that I've read it more thoroughly I see what you're trying to do. 👍

bacongobbler
bacongobbler reviewed Jul 18, 2016
View reviewed changes
cmd/apps.go
fmt.Printf("=== %s Application\n", app.ID)
fmt.Println("updated: ", app.Updated)
fmt.Println("uuid: ", app.UUID)
fmt.Println("created: ", app.Created)
fmt.Println("url: ", app.URL)
fmt.Println("url: ", url)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we shouldn't be changing this because the server fills out this field for us. If the server is returning us false information when the app has no domain, then we should fix this in the controller.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, this adds an additional API request to this command which we try to avoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't from the controller.... it's from the SDK, which is making the URL assumption of appname.cluster.com

Copy link
Contributor Author

@Joshua-Anderson Joshua-Anderson Jul 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intend to remove app.URL from the SDK after this is merged.

@Joshua-Anderson
Copy link
Contributor Author

@bacongobbler Fixed the issues you brought up!

arschles
arschles reviewed Jul 18, 2016
View reviewed changes
cmd/apps.go
@@ -276,3 +284,33 @@ func AppTransfer(appID, username string) error {

return nil
}

var errNoDomain error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove this error from package-global scope? if it needs to be package-global, then make it a struct

@arschles
Copy link
Member

moving out of the 2.2 milestone

@arschles arschles removed this from the v2.2 milestone Jul 19, 2016
@Joshua-Anderson
Copy link
Contributor Author

@arschles I refactored the code to instead check for a empty string rather than an error, removing the need to a package level error.

@arschles arschles added the LGTM1 label Jul 19, 2016
@Joshua-Anderson Joshua-Anderson merged commit 75c43e7 into deis:master Jul 25, 2016
@Joshua-Anderson Joshua-Anderson deleted the apps-open branch July 25, 2016 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Joshua-Anderson Joshua-Anderson

Labels
enhancement LGTM1 LGTM2 waiting for ci
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Joshua-Anderson @deis-bot @bacongobbler @arschles