-
Notifications
You must be signed in to change notification settings - Fork 16
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 keyFilePath, emulatorHost and projectId as configuration parameters #6
base: master
Are you sure you want to change the base?
add keyFilePath, emulatorHost and projectId as configuration parameters #6
Conversation
* describe the symfony 5.3 behaviour * document new configuration options * replace deprecated GCLOUD_PROJECT environment variable in documentation * resolve additional options as environment variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @ricktap for your work! I have som proposes but generally I agree with your PR.
|
||
const GOOGLE_APPLICATION_CREDENTIALS = 'GOOGLE_APPLICATION_CREDENTIALS'; | ||
const GOOGLE_CLOUD_PROJECT = 'GOOGLE_CLOUD_PROJECT'; | ||
const PUBSUB_EMULATOR_HOST = 'PUBSUB_EMULATOR_HOST'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please use explicit constant access visibility? I also propose to rename these contants as it's not obvious what they represent.
public const KEY_FILE_PATH_ENV_NAME = 'GOOGLE_APPLICATION_CREDENTIALS';
public const PROJECT_ID_ENV_NAME = 'GOOGLE_CLOUD_PROJECT';
public const EMULATOR_HOST_ENV_NAME = 'PUBSUB_EMULATOR_HOST';
'projectId' => self::GOOGLE_CLOUD_PROJECT, | ||
'emulatorHost' => self::PUBSUB_EMULATOR_HOST, | ||
'keyFilePath' => self::GOOGLE_APPLICATION_CREDENTIALS | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to put it into private constant as:
private const ENV_OPTIONS_MAP = [
'projectId' => self::PROJECT_ID_ENV_NAME,
'emulatorHost' => self::EMULATOR_HOST_ENV_NAME,
'keyFilePath' => self::KEY_FILE_PATH_ENV_NAME,
];
```php | ||
(new Dotenv())->usePutenv()->... | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this whole part about using ->usePutenv
can be deleted. By this PR we are going to force use of putenv by resolving gps transport options and it's ok, because Google library can only work with globaly accessible env vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to use env vars anymore. You can pass all these options in when you instantiate the PubSubClient.
This is pretty much what I did when I forked this project over to https://github.com/chrishemmings/gps-messenger-bundle
Nice PR. @ricktap Any updates on it? |
adds keyFilePath, emulatorHost and projectId as described in #5