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

Implement network-profiles describe feature #964

Merged
merged 6 commits into from
Aug 11, 2020

Conversation

pawelpasterz
Copy link
Contributor

Fixes #963

Test Plan

How do we know the code works?

Run flank network-profiles describe LTE and see the result:

downRule:
  bandwidth: 16000.0
  delay: 0.040s
  packetLossRatio: 0.001
id: LTE
upRule:
  bandwidth: 16000.0
  delay: 0.040s
  packetLossRatio: 0.001

Compare it with gcloud alpha firebase test network-profiles describe LTE output

Checklist

  • Documented
  • Unit tested
  • release_notes.md updated

@pawelpasterz pawelpasterz self-assigned this Aug 9, 2020
adamfilipow92
adamfilipow92 previously approved these changes Aug 10, 2020
defaultValue = "",
description = ["The network profile to describe, found" +
" using \$ gcloud beta firebase test network-profiles list."])
var profile: String = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

profile-> profileId


private const val UNABLE = "[Unable to fetch]"

private fun Any?.toStringOrEmpty() = this?.toString() ?: UNABLE
Copy link
Contributor

Choose a reason for hiding this comment

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

toStringOrEmpty -> toStringOrUnableToFetch or toStringOrUnable

piotradamczyk5
piotradamczyk5 previously approved these changes Aug 10, 2020
Copy link
Contributor

@jan-goral jan-goral left a comment

Choose a reason for hiding this comment

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

I guess we should use continuation indent from the official style guide.
https://kotlinlang.org/docs/reference/code-style-migration-guide.html

@piotradamczyk5
Copy link
Contributor

I guess we should use continuation indent from the official style guide.
https://kotlinlang.org/docs/reference/code-style-migration-guide.html

Maybe we all should use ktlint as default formatter

@jan-goral
Copy link
Contributor

I guess we should use continuation indent from the official style guide.
https://kotlinlang.org/docs/reference/code-style-migration-guide.html

Maybe we all should use ktlint as default formatter

I have no experience with ktlint. We can discuss it with the rest of the team. Anyway, I guess setting correct indentation is easier than setting up a new tool and will prevent most of the unnecessary changes after auto-formatting.

paramLabel = "PROFILE_ID",
defaultValue = "",
description = ["The network profile to describe, found" +
" using \$ gcloud beta firebase test network-profiles list."])
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's should be

gcloud beta firebase test network-profiles describe

?

Copy link
Contributor Author

@pawelpasterz pawelpasterz Aug 11, 2020

Choose a reason for hiding this comment

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

Nah, the description states that we need to provide profileId which we can get from gcloud beta firebase test network-profiles list. So it's like -- 'first find id from the list (using this command) and then pass it as parameter'

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh, ok thanks!

@pawelpasterz
Copy link
Contributor Author

I guess we should use continuation indent from the official style guide.
https://kotlinlang.org/docs/reference/code-style-migration-guide.html

Maybe we all should use ktlint as default formatted

👍 I am for linter.
Well in context of this PR it's easier to make requested indentation change manually, but we definitely need to implement some solution

@piotradamczyk5
Copy link
Contributor

I guess we should use continuation indent from the official style guide.
https://kotlinlang.org/docs/reference/code-style-migration-guide.html

Maybe we all should use ktlint as default formatted

👍 I am for linter.
Well in context of this PR it's easier to make requested indentation change manually, but we definitely need to implement some solution

Installation is super easy:

brew install ktlint
ktlint applyToIDEA  // this will add code style to intellij

and later on
ktlint - show all issues
ktlint -F -show and fix all issue

@piotradamczyk5 piotradamczyk5 self-requested a review August 11, 2020 10:05
@pawelpasterz pawelpasterz requested a review from jan-goral August 11, 2020 10:19
@pawelpasterz pawelpasterz merged commit c3686e5 into master Aug 11, 2020
@pawelpasterz pawelpasterz deleted the add-profiles-describe-command branch August 11, 2020 10:37
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.

Implement network-profiles describe feature
4 participants