Skip to content

Conversation

jamiehannaford
Copy link
Contributor

Partially addresses #422

@ycombinator
Copy link
Contributor

The Travis build failed on the PSR-2 check. You can make the necessary fixes by running the following from the project's root directory:

$ vendor/bin/php-cs-fixer fix --level psr2 .

Copy link
Contributor

Choose a reason for hiding this comment

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

'{username}' should be replaced by something like getenv('RS_IDENTITY_USER_NAME') so the user needs only use one mechanism (environment variables) to pass in information needed to execute these scripts.

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'm actually not a big fan of asking users to set a whole bunch of environment variables. For me, that's more work than tweaking an inline variable. Plus on DRC we use inline placeholders rather than env vars, so there's added inconsistency if we do something different.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with using inline placeholders instead of environment variables but three considerations come to mind:

  1. There are several existing samples (in the samples/ directory) that would need updating,
  2. I would rather go inline placeholders all the way than having some values specified via environment variables and others via inline placeholdes,
  3. Will inline placeholders work for @maxlinc's scripts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'm fine with that. That's a good point about @maxlinc's scripts, though - I'll ping Max and ask what his requirements are to make it work. If inline placeholders are fine, I'll go ahead and change all the other samples and submit in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

If inline placeholders are fine, I'll go ahead and change all the other samples and submit in another PR.

@jamiehannaford I've created #467 to track progress with changing all the other code samples so we are out of our current inconsistent state (some samples use getenv(...) while others use {...}).

@jamiehannaford
Copy link
Contributor Author

@ycombinator This is ready for another review too 😃

ycombinator added a commit that referenced this pull request Nov 19, 2014
@ycombinator ycombinator merged commit 7553f37 into rackspace:working Nov 19, 2014
@jamiehannaford jamiehannaford deleted the identity-samples branch November 21, 2014 09:01
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.

2 participants