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

Add keystone support #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add keystone support #12

wants to merge 2 commits into from

Conversation

sunloverz
Copy link

No description provided.

@sunloverz
Copy link
Author

@thousandsofthem @pyeremenko could you please review

@@ -421,6 +443,32 @@ protected function setCommonHeaders()
'Keep-Alive' => '300'
);
}

protected function getToken()
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find any call of getToken :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry, it's iron_core

@pyeremenko
Copy link
Contributor

@thousandsofthem, please, look at this PR. I think it could be merged.


$req = array(
'auth' => array(
'tenantName' => $this->keystone['tenant'],
Copy link
Contributor

Choose a reason for hiding this comment

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

tenantId as far as I understand how it works , names are not reliable

Copy link
Contributor

Choose a reason for hiding this comment

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

@thousandsofthem, could you clarify?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

P.S. Also, tenantId always equal to project_id so could be derived from it
P.P.S. tenantName is not unique therefore can't be used as a key

Copy link
Contributor

Choose a reason for hiding this comment

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

@thousandsofthem,

  • @edsrzf said we should support not tenantId, but tenantName, please discuss it with him.
  • Why 'tenantName' string is not unique key?

Copy link
Contributor

Choose a reason for hiding this comment

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

tenantName means "project name", and user can set any name he wants to, including duplicate ones.
@edsrzf do you know anything else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Some notes:

  • only project_id required (with tenantId), project name is extra dependency
  • tenantId actually works with test server and on AT&T installation

Copy link
Contributor

Choose a reason for hiding this comment

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

any update so far?

Copy link
Contributor

Choose a reason for hiding this comment

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

@edsrzf, @carimura, @treeder

We implemented support of format "keystone": { "server": ..., "tenant": ..., "username": ..., "password": ... } because we discussed it with Evan and he accepted this way. @thousandsofthem wants to change this format and we will change it if somebody of you confirm it.

@rjocoleman
Copy link

I know this is WIP but it's out of sync with https://github.com/iron-io/iron_mq_php/tree/v3

Perhaps a note in the iron-io/iron_mq_php v3 branch README would be appropriate?

Specific issue is Notice: Undefined property: IronMQ::$use_keystone, it's easy enough to patch to workaround but still an issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants