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

AL-566 Change API host to terminus.pantheon.io #1180

Merged
merged 1 commit into from
Sep 12, 2016
Merged

Conversation

bensheldon
Copy link
Contributor

  • Changes Terminus client to talk to terminus.pantheon.io host
  • Adds a DASHBOARD_HOST constant for where the Dashboard URL is referenced in interface

@pantheon-mentionbot
Copy link

@bensheldon, thanks for your PR! By analyzing the blame information on this pull request, we identified @TeslaDethray, @ronan and @ari-gold to be potential reviewers

@ronan
Copy link
Contributor

ronan commented Sep 8, 2016

I don't see why we need the dashboard host and protocol separated only to join them again later but since this is the established pattern I care probably 2/10 about that.

+1

@coveralls
Copy link

coveralls commented Sep 8, 2016

Coverage Status

Coverage decreased (-0.02%) to 13.984% when pulling 050a372 on terminus_host into b588e6b on master.

@coveralls
Copy link

coveralls commented Sep 8, 2016

Coverage Status

Coverage decreased (-0.02%) to 13.984% when pulling 050a372 on terminus_host into b588e6b on master.

@@ -289,9 +289,9 @@
-
request:
method: POST
url: 'https://dashboard.pantheon.io/api/authorize/machine-token'
Copy link
Contributor

Choose a reason for hiding this comment

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

I am sad that I missed erasing the machine token from this fixture.

@TeslaDethray
Copy link
Contributor

+1

TERMINUS_PORT: 443
TERMINUS_PROTOCOL: 'https'
TERMINUS_SSH_HOST: null

# User Prompts
DASHBOARD_HOST: 'dashboard.pantheon.io'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this only used for the site dashboard command ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it for the prompt that tells you where to go to create a machine token. I missed the site dashboard command. I'll search again, but anywhere else it links to something on the Dashboard dashboard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for reference, I'm grepping through with config.*host as there seems to be 2 styles for referencing config vars:

$config = Config::getAll();
$config['host']

And

Config::get('host')

Not sure if there is a style preference for one over the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bensheldon OK. Cool. I think it's just site dashboard and instructions for machine token generation.

Copy link
Member

Choose a reason for hiding this comment

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

@bensheldon The later is just a wrapper for the former. I would say that the later should be preferred.

@coveralls
Copy link

coveralls commented Sep 12, 2016

Coverage Status

Coverage decreased (-0.02%) to 13.982% when pulling 76eb94f on terminus_host into bafdb43 on master.

@bensheldon bensheldon merged commit 090812e into master Sep 12, 2016
@bensheldon bensheldon deleted the terminus_host branch September 12, 2016 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants