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

Update project-id to project-number #421

Merged

Conversation

fran-tirapu
Copy link
Contributor

@fran-tirapu fran-tirapu commented Aug 2, 2024

QA

Previous project-id and new project-number arguments are used in couple of use cases and behaviour is the same after the name change:

  •  If argument is not passed on previous version, --project-id is requested as mandatory:
$> firebase-app-distribution get-latest-build-version                                                                                
usage: firebase-app-distribution get-latest-build-version [-h] [--log-stream {stderr,stdout}] [--no-color] [--version] [-s] [-v] --app-id APP_ID
                                                          --project-id PROJECT_ID [--credentials CREDENTIALS]
firebase-app-distribution get-latest-build-version: error: the following arguments are required: --app-id/-a, --project-id/-p

$> firebase-app-distribution releases list                                                                                        
usage: firebase-app-distribution releases list [-h] [--log-stream {stderr,stdout}] [--no-color] [--version] [-s] [-v] --app-id APP_ID
                                               [--limit LIMIT] [--order-by {createTimeDesc,createTime}] [--json] --project-id PROJECT_ID
                                               [--credentials CREDENTIALS]
firebase-app-distribution releases list: error: the following arguments are required: --app-id/-a, --project-id/-p

Now same output is returned but with --project-number/-p

  • Several test has been done to obtain same response from Firebase API without success, because you need a valid iOS or Android app published to obtain a valid http response 200. At least we can test that final URL composition is the same and returned http response error code is also the same.
$> firebase-app-distribution get-latest-build-version --project-number 123456 --app-id 654321

GET https://firebaseappdistribution.googleapis.com/v1/projects/123456/apps/654321/releases?orderBy=createTimeDesc&pageSize=1&alt=json --> HttpError 400 Request contains an invalid argument.
$> firebase-app-distribution releases list --project-number 123456 --app-id 654321 

GET https://firebaseappdistribution.googleapis.com/v1/projects/123456/apps/654321/releases?orderBy=createTimeDesc&pageSize=25&alt=json --> HttpError 400 Request contains an invalid argument.

@fran-tirapu fran-tirapu marked this pull request as ready for review August 2, 2024 10:33
@artemii-yanushevskyi
Copy link
Contributor

For backwards compatibility, please keep --project-id flag, and project_id argument in public Python API. If the deprecated flag or the argument is used, a deprecation message should be shown.

Copy link
Contributor

@priitlatt priitlatt left a comment

Choose a reason for hiding this comment

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

Exactly what Artemii said regarding backwards compatibility. It would be OK and even preferred to only show the current configuration option in Markdown docs and terminal help messages, but the old version must remain functional for the time being. Otherwise we're going to break workflows that are working fine as of now.

@fran-tirapu
Copy link
Contributor Author

The job done on #422 give us the new option mutually_exclusive_group.
Docs are generated properly and arguments are automatically checked by argparse standard lib.

Copy link
Contributor

@priitlatt priitlatt left a comment

Choose a reason for hiding this comment

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

Backwards compatibility must be retained both on Python API and CLI level and warning messages should reflect the actual misconfiguration. Currently Python API is not backwards compatible and all deprecation warnings are about CLI.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
src/codemagic/tools/firebase_app_distribution/arguments.py Outdated Show resolved Hide resolved
src/codemagic/tools/firebase_app_distribution/arguments.py Outdated Show resolved Hide resolved
Copy link
Contributor

@priitlatt priitlatt left a comment

Choose a reason for hiding this comment

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

🚀

@fran-tirapu fran-tirapu merged commit a09da0c into master Aug 16, 2024
11 checks passed
@fran-tirapu fran-tirapu deleted the 420-firebase-project-number-is-referred-to-as-project-id branch August 16, 2024 14:12
@priitlatt priitlatt linked an issue Aug 16, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

firebase-app-distribution's Project ID to Project Number
3 participants