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

flank auth login #449

Merged
merged 1 commit into from
Jan 10, 2019
Merged

flank auth login #449

merged 1 commit into from
Jan 10, 2019

Conversation

bootstraponline
Copy link
Contributor

@bootstraponline bootstraponline commented Jan 9, 2019

Fix #398

Flank now allows you to login as a user. Type flank auth login and follow the web prompt. User auth is an alternative to using a service account.

@bootstraponline bootstraponline force-pushed the gcloud_auth branch 3 times, most recently from 5394c79 to 64deb6f Compare January 10, 2019 19:01
@Flank Flank deleted a comment from codecov-io Jan 10, 2019
@bootstraponline bootstraponline changed the title WIP | Gcloud auth flank auth login Jan 10, 2019
@codecov-io
Copy link

codecov-io commented Jan 10, 2019

Codecov Report

Merging #449 into master will decrease coverage by 0.76%.
The diff coverage is 49.01%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #449      +/-   ##
============================================
- Coverage     79.32%   78.55%   -0.77%     
- Complexity      630      640      +10     
============================================
  Files            69       73       +4     
  Lines          1790     1833      +43     
  Branches        271      272       +1     
============================================
+ Hits           1420     1440      +20     
- Misses          195      217      +22     
- Partials        175      176       +1

@@ -36,6 +39,10 @@ class Main : Runnable {
private var printVersion = false

companion object {
init {
// GoogleApiLogger.logAllToStdout()
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 left here for debugging purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think it'll be cleaned up as part of #446

import java.io.IOException

// https://github.com/googleapis/google-oauth-java-client
// Code from: https://developers.google.com/sheets/api/quickstart/java
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd help to add some context to the comment (like why you chose to copy from sheets)

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 updated the comment. The usage of GoogleAuthorizationCodeFlow is the same for any Google service. When the API changes, Google updates their official reference documentation.

HTTP_TRANSPORT = GoogleNetHttpTransport.newTrustedTransport()
DATA_STORE_FACTORY = FileDataStoreFactory(CRED_FOLDER)
} catch (t: Throwable) {
t.printStackTrace()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is copied code or not, but why not let the exception propagate or just rethrow it as a RuntimeException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. Updated.

}
}

fun activateUserAuth(): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing kdoc. Also, it seems strange that this would return a boolean (based on the naming of the method).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to hasUserAuth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kdoc isn't used at the moment on the methods. A PR will be adding detekt support soon. At that point we may decide to enforce function docs.

val CRED = File(CRED_FOLDER, "StoredCredential")

// https://github.com/bootstraponline/gcloud_cli/blob/40521a6e297830b9f652a9ab4d8002e309b4353a/google-cloud-sdk/platform/gsutil/gslib/utils/system_util.py#L177
private val clientId = "32555940559.apps.googleusercontent.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it by design that these are hardcoded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, they're hardcoded in gcloud CLI as well.

@bootstraponline bootstraponline merged commit f710c18 into master Jan 10, 2019
@bootstraponline bootstraponline deleted the gcloud_auth branch January 10, 2019 21:54
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.

3 participants