composer/rest/get_client_id.py has has issues on giving arguments

[ricconoel]

In which file did you encounter the issue?

composer/rest/get_client_id.py

Did you change the file? If so, how?

Original:

    parser.add_argument('project_id', help='Your Project ID.')
    parser.add_argument(
        'location', help='Region of the Cloud Composer environent.')
    parser.add_argument(
        'composer_environment', help='Name of the Cloud Composer environent.')

Fixed locally:

    parser.add_argument('--project_id', help='Your Project ID.')
    parser.add_argument(
        '--location', help='Region of the Cloud Composer environent.')
    parser.add_argument(
        '--composer_environment', help='Name of the Cloud Composer environment.')

Describe the issue

When running script error shows

usage: id.py [-h] project_id location composer_environment
id.py: error: the following arguments are required: project_id, location, composer_environment

Needs to add '--' on the arguments. Issue was encountered here https://stackoverflow.com/questions/66400667

[leahecole]

Hey! Thanks for filing this! I suspect you are also Ricco D, the first answer on it, and if that's true, thank you for that as well! I added an additional answer. For anyone too lazy to flip back and forth between StackOverflow and here, the tl;dr is

• This script is not intuitive
• error handling for the arg parsing isn't intuitive
• Leah will reach out to the samples WG for best practices and how to fix it - it may mean removing the arg parse section altogether to be in line with our other samples, or improving the argparsing

[scouvreur]

Hey @leahecole - is there any work pending to do on this ? If so would be happy to help !

[leahecole]

Hey @scouvreur ! This is in my forever backlog but let me fill you and any future readers in on where I'm at

Long term vision (probably not before the end of Q2):

• I'm working with some colleagues to make sure that this sample, which is a few years old, both maintains usefulness for folks who use it and also is embodying best practices for maintainability on our end to make sure that it's well tested, aligned with what we do for other languages where appropriate, and easy to keep working for as long as it's relevant to Composer users. Passing command line arguments is something we're still talking about. Those discussions are ongoing and might take awhile.

Short term plan (either already done or ideally will be done before end of Q2):

• I've added comments to the script on Github above the if __name__ == "__main__" line with example usage.
• I'm working with another colleague on a cleanup of the tutorial this script was originally written for and have a noted action item to make sure usage is clearly spelled out both inline with comments and in that tutorial

Added bonus

• Colleague shared a cURL command that's an alternative

curl -v AIRFLOW_URL 2>&1 >/dev/null | grep -o "client_id\=[A-Za-z0-9-]*\.apps\.googleusercontent\.com"

where AIRFLOW_URL is the URL of your Airflow webserver

It'll return something like client_id=123456789-abc123defghi456jkl789.apps.googleusercontent.com. Whereas this script would just give you 123456789-abc123defghi456jkl789.apps.googleusercontent.com I wish it didn't also return the client_id=, but I was falling down a regex rabbit hole and learning the limitations of the regex used by the version of grep I have on my machine and decided including the client_id= was not the worst thing in the world 😁

[scouvreur]

Awesome @leahecole - thanks for the detailed explanation ! I will open a PR with some improvements you proposed and maybe we can continue the discussion there ?