Skip to content

feat: add locales description command for ios and android#969

Merged
adamfilipow92 merged 15 commits into
masterfrom
add-locales-describe-command
Aug 14, 2020
Merged

feat: add locales description command for ios and android#969
adamfilipow92 merged 15 commits into
masterfrom
add-locales-describe-command

Conversation

@adamfilipow92
Copy link
Copy Markdown
Contributor

@adamfilipow92 adamfilipow92 commented Aug 10, 2020

Fixes #966

Test Plan

How do we know the code works?

When run


     flank firebase test android|ios locales describe LOCALE_ID

flank should return output like gcloud cli when execute


gcloud alpha firebase test android|ios locales describe LOCALE_ID


example for flank firebase test android locales describe en


id: en
name: English
tags:
- default

example for flank firebase test android locales describe pl


id: pl
name: Polish

example for the wrong LOCALE_ID flank firebase test android locales describe test


ERROR: 'test' is not a valid locale

Checklist

  • Unit tested
  • release_notes.md updated

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #969 into master will increase coverage by 0.34%.
The diff coverage is 88.46%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #969      +/-   ##
============================================
+ Coverage     81.28%   81.62%   +0.34%     
- Complexity      673      687      +14     
============================================
  Files           206      211       +5     
  Lines          3746     3816      +70     
  Branches        569      581      +12     
============================================
+ Hits           3045     3115      +70     
  Misses          395      395              
  Partials        306      306              

@adamfilipow92 adamfilipow92 force-pushed the add-locales-describe-command branch from 769975d to 27a10ab Compare August 11, 2020 13:06
@adamfilipow92 adamfilipow92 marked this pull request as ready for review August 11, 2020 13:37
Comment thread release_notes.md Outdated
- [#970](https://github.com/Flank/flank/pull/970) Fixing Flood of snapshot releases. ([piotradamczyk5](https://github.com/piotradamczyk5))
- [#972](https://github.com/Flank/flank/pull/972) Fix printing release version. ([piotradamczyk5](https://github.com/piotradamczyk5))
- [#964](https://github.com/Flank/flank/pull/964) Implement network-profiles describe feature. ([pawelpasterz](https://github.com/pawelpasterz))
-
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not needed :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed thanks :)

locale.tags.filterNotNull().forEach { tag -> appendln("- $tag") }
}.toString().trim()

private fun String?.orErrorMessage(locale: String) = this ?: "ERROR: '$locale' is not a valid locale".trimIndent()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that trimIndent() is not needed for this simple string message

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Changed

Copy link
Copy Markdown
Contributor

@pawelpasterz pawelpasterz left a comment

Choose a reason for hiding this comment

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

For flank firebase test android locales describe zu_ZA flank is missing region information


fun getLocaleDescription(projectId: String, locale: String) = getLocales(projectId).getLocaleDescription(locale)

private fun getLocales(projectId: String) = deviceCatalog(projectId).runtimeConfiguration.locales
Copy link
Copy Markdown
Contributor

@pawelpasterz pawelpasterz Aug 12, 2020

Choose a reason for hiding this comment

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

I am wondering why do we need projectId to make this request? I mean, I can see from the code that we use it to set a property for request under the hood, but why since it is publicly available information... maybe it's due to 'default' tag...I don't know and just thinking :)

Copy link
Copy Markdown
Contributor

@piotradamczyk5 piotradamczyk5 Aug 12, 2020

Choose a reason for hiding this comment

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

Probably device catalogue could be different for different project ids, so API needs this to return catalogue which is available for current projectID.

Copy link
Copy Markdown
Contributor Author

@adamfilipow92 adamfilipow92 Aug 12, 2020

Choose a reason for hiding this comment

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

@pawelpasterz requested change done. Thanks for the tests. I check if projectID is really required and get feedback
Edit:
We can use empty projectID and result for ios/android is the same but here we loading projectID from selected config or from the default so I think it's ok. I think every flank users have a test-lab project.

WDYT? @pawelpasterz @piotradamczyk5

Copy link
Copy Markdown
Contributor

@piotradamczyk5 piotradamczyk5 Aug 12, 2020

Choose a reason for hiding this comment

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

As per documenation:

`--project` and its fallback` core/project` property play two roles in the invocation. It specifies the project of the resource to operate on. 

It has a fallback option, so if the project is not specified in the environmental variable and not provided by config for me it is ok to use empty

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hey, we need projectId

the reason is select projects have access to secret device catalogs 🙂 These catalogs are not available to the public.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably device catalogue could be different for different project ids,

this is correct!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add a comment that explains why projectId is sent to the device catalog

piotradamczyk5
piotradamczyk5 previously approved these changes Aug 12, 2020
@adamfilipow92 adamfilipow92 changed the title Add locales description command for ios and android feat: add locales description command for ios and android Aug 12, 2020
Copy link
Copy Markdown
Contributor

@pawelpasterz pawelpasterz left a comment

Choose a reason for hiding this comment

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

I know I am super annoying but let's also handle those ugly % chars at the end of line
Screenshot 2020-08-12 at 18 06 50
I don't know if this is only me but gcloud does not print them

@adamfilipow92
Copy link
Copy Markdown
Contributor Author

adamfilipow92 commented Aug 12, 2020

I know I am super annoying but let's also handle those ugly % chars at the end of line
Screenshot 2020-08-12 at 18 06 50
I don't know if this is only me but gcloud does not print them

Hmm, I don't see this when I testing. You have right it's should be fixed thanks! I check and fix it.

pawelpasterz
pawelpasterz previously approved these changes Aug 13, 2020
Copy link
Copy Markdown
Contributor

@pawelpasterz pawelpasterz left a comment

Choose a reason for hiding this comment

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

For incorrect locale, gcloud ends run with exit code 1 where flank's s 0. I think we should also reflect this.

@adamfilipow92
Copy link
Copy Markdown
Contributor Author

For incorrect locale, gcloud ends run with exit code 1 where flank's s 0. I think we should also reflect this.

Thanks, changed 👍

pawelpasterz
pawelpasterz previously approved these changes Aug 13, 2020
@adamfilipow92 adamfilipow92 merged commit e633445 into master Aug 14, 2020
@adamfilipow92 adamfilipow92 deleted the add-locales-describe-command branch August 14, 2020 14:22
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 locales describe feature

6 participants