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.

Sign up for GitHub

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

Jump to bottom

fix: Print Flank version only on run command #1585

Merged
merged 7 commits into from
Feb 12, 2021
Merged

fix: Print Flank version only on run command #1585

merged 7 commits into from
Feb 12, 2021

Conversation

adamfilipow92
Copy link
Contributor

@adamfilipow92 adamfilipow92 commented Feb 10, 2021 •
edited
Loading

Fixes #1561

Test Plan

How do we know the code works?

I case

  1. Run android or ios test
  2. Flank should print version, revision, and sessionId

II case

  1. Run Flank with --version
  2. Flank should print version, revision, and sessionId

III case

  1. Run a command like ```flank provided-software list````
  2. Version information should not be printed.

@adamfilipow92 adamfilipow92 self-assigned this Feb 10, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2021 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@adamfilipow92 adamfilipow92 changed the title initial solution fix: Print flank version only with run command Feb 10, 2021
@adamfilipow92 adamfilipow92 changed the title fix: Print flank version only with run command fix: Print Flank version only on run command Feb 10, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2021 •
edited
Loading

Timestamp: 2021-02-12 15:49:47
Buildscan url for ubuntu-workflow run 561461420
https://gradle.com/s/vloxotnfmtofo

Sloox
Sloox reviewed Feb 10, 2021
View reviewed changes
test_runner/src/main/kotlin/ftl/util/Utils.kt Outdated
@@ -68,6 +69,13 @@ private fun getResource(name: String): InputStream {
?: throw FlankGeneralError("Unable to find resource: $name")
}

fun printVersionInfo() {
logLn("version: " + readVersion())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logLn("version: " + readVersion())
logLn("version:${readVersion}")

?
The same for all 2 lines below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Changed!

Sloox
Sloox suggested changes Feb 10, 2021
View reviewed changes
test_runner/src/main/kotlin/ftl/cli/firebase/test/android/AndroidRunCommand.kt Outdated
@@ -59,6 +60,9 @@ class AndroidRunCommand : CommonRunCommand(), Runnable {
}

override fun run() {

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm do you have git pre commit working? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but pre commit hook not catch that. I removed this line! Thanks!

@adamfilipow92 adamfilipow92 marked this pull request as ready for review February 10, 2021 17:50
@Sloox Sloox self-requested a review February 11, 2021 09:04
@pawelpasterz
Copy link
Contributor

pawelpasterz commented Feb 11, 2021 •
edited
Loading

It would be great to have a couple of tests to verify it, do you think it requires lots of work to implement it?

@adamfilipow92
Copy link
Contributor Author

It would be great to have a couple of tests to verify it, do you think it requires lots of work to implement it?

Right! Tests added! 👍

@bootstraponline bootstraponline force-pushed the 1561-print-flank-version-only-on-run-command branch from e796e35 to 15ac48c Compare February 11, 2021 16:59
piotradamczyk5
piotradamczyk5 approved these changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@piotradamczyk5 piotradamczyk5 left a comment

Choose a reason for hiding this comment

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

Tested few commands (flank ios versions list, flank android models list, flank network-profiles list) and middle command (help of groups) - version is not printed
Tested (flank -v , flank android run, flank ios run) - Version is printed

Code looks good 👍

jan-goral
jan-goral suggested changes Feb 12, 2021
View reviewed changes
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 love that now we are absolutely sure that the version will not be printed in other commands than run, but IMO from a maintenance perspective, it's a bad idea to have a test for checking that something wasn't occurring. Do you know what I mean? There is an infinite number of things that cannot occur in flank runtime, we shouldn't focus on them.

@bootstraponline bootstraponline force-pushed the 1561-print-flank-version-only-on-run-command branch from 795b715 to b616b0a Compare February 12, 2021 15:09
@mergify mergify bot merged commit 4247325 into master Feb 12, 2021
@mergify mergify bot deleted the 1561-print-flank-version-only-on-run-command branch February 12, 2021 15:53
@github-actions github-actions bot locked and limited conversation to collaborators Feb 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Sloox Sloox Sloox approved these changes

@jan-goral jan-goral jan-goral approved these changes

@piotradamczyk5 piotradamczyk5 piotradamczyk5 approved these changes

@pawelpasterz pawelpasterz Awaiting requested review from pawelpasterz

@zuziaka zuziaka Awaiting requested review from zuziaka

Assignees

@adamfilipow92 adamfilipow92

Labels
Bug P1 Tiger 🐯
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Only print flank version on Run command
5 participants
@adamfilipow92 @pawelpasterz @Sloox @jan-goral @piotradamczyk5