search

dart-archive / googleapis_auth

Obtain OAuth 2.0 credentials to access Google APIs
https://pub.dev/packages/googleapis_auth
BSD 3-Clause "New" or "Revised" License
38 stars 26 forks source link

Look for GCE metadata host in environment #58

Closed joshuaseaton closed 5 years ago

joshuaseaton commented 5 years ago

This change updates the googleapis_auth dart package to look for the metadata host first at $GCE_METADATA_HOST. This simplifies integration testing with credentialed services and follows the suit of golang and python who have implemented the same check (e.g., https://github.com/googleapis/google-cloud-go/blob/1310e431bb0f7a8662a328dab8983aee4c5869e4/compute/metadata/metadata.go#L41-L46).

joshuaseaton commented 5 years ago

@jonasfj, as discussed earlier with @mkustermann.

joshuaseaton commented 5 years ago

This needs a changelog entry and ideally an update to the doc comment on the function!

Done. Please take another look.

joshuaseaton commented 5 years ago

How should I interpret https://travis-ci.org/dart-lang/googleapis_auth/jobs/557493336? (Or should I maybe ignore it altogether?)

kevmoo commented 5 years ago

How should I interpret https://travis-ci.org/dart-lang/googleapis_auth/jobs/557493336? (Or should I maybe ignore it altogether?)

You'll need to do platform-specific import here, since this code you're adding breaks web stuff.

joshuaseaton commented 5 years ago

May you please point to an example of a platform-specific import and specifically how one detects "is web"?

On Thu, Jul 11, 2019 at 12:24 PM Kevin Moore notifications@github.com wrote:

How should I interpret https://travis-ci.org/dart-lang/googleapis_auth/jobs/557493336? (Or should I maybe ignore it altogether?)

You'll need to do platform-specific import here, since this code you're adding breaks web stuff.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dart-lang/googleapis_auth/pull/58?email_source=notifications&email_token=AEDQ6QI3XJAZ2AGL3FCMTTTP66CFPA5CNFSM4IBP5BN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZXXH3Y#issuecomment-510620655, or mute the thread https://github.com/notifications/unsubscribe-auth/AEDQ6QI5SVOBZYC7N7MAKSDP66CFPANCNFSM4IBP5BNQ .

natebosch commented 5 years ago

Given the comment about running only on the VM I wonder if test/oauth2_flows/metadata_server_test.dart should have a @TestOn which also limits it to the VM? Someone else with more familiarity with the design here could weigh in on whether that would be considered breaking.

@joshuaseaton - a rough sketch if we do decide we need platform specific approaches would be:

// in the file you edited...
import 'metadata_host_stub.dart'
  if (dart.library.io) 'metadata_host_io.dart';

...

// instead of Platform.environment
final metadataHost = findMetadataHost();
// metadata_host_stub.dart
String findMetadataHost() => 'metadata';
// metadata_host_io.dart
import 'dart:io';

String findMetadataHost() => Platform.environment[_GCE_METADATA_HOST_ENV_VAR] ?? 'metadata';
joshuaseaton commented 5 years ago

Done. Thanks for spelling that out.

On Thu, Jul 11, 2019 at 1:15 PM Nate Bosch notifications@github.com wrote:

Given the comment about running only on the VM I wonder if test/oauth2_flows/metadata_server_test.dart should have a @TestOn which also limits it to the VM? Someone else with more familiarity with the design here could weigh in on whether that would be considered breaking.

@joshuaseaton https://github.com/joshuaseaton - a rough sketch if we do decide we need platform specific approaches would be:

// in the file you edited...import 'metadata_host_stub.dart' if (dart.library.io) 'metadata_host_io.dart';

... // instead of Platform.environmentfinal metadataHost = findMetadataHost();

// metadata_host_stub.dartString findMetadataHost() => 'metadata';

// metadata_host_io.dartimport 'dart:io'; String findMetadataHost() => Platform.environment[_GCE_METADATA_HOST_ENV_VAR] ?? 'metadata';

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/googleapis_auth/pull/58?email_source=notifications&email_token=AEDQ6QMRR42H3WD4G62UPMDP66IHPA5CNFSM4IBP5BN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZX3MMI#issuecomment-510637617, or mute the thread https://github.com/notifications/unsubscribe-auth/AEDQ6QNPSZYHWB72YX7QJP3P66IHPANCNFSM4IBP5BNQ .

joshuaseaton commented 5 years ago

(Apologies for the multiple pushes. Still setting up my environment - and learning to write dart like a newly birthed foal learns how to walk.) Checks now pass.