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

{AzureContainerApp} az containerapp create --bind "my-app" throws error when service has dashes in the name "-" #6530

Closed
wants to merge 5 commits into from

Conversation

navba-MSFT
Copy link
Contributor

fixes #6524

Container Apps lets me create a container with dashes ("-") in the name, but then I can't reference that container in the --bind parameter. so I expect the command to accept the service with a dash in the name because I was allowed to create it in the first place.

cli.azure.cli.core.azclierror: Traceback (most recent call last):
  File "D:\a\_work\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\knack/cli.py", line 233, in invoke
  File "D:\a\_work\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\azure/cli/core/commands/__init__.py", line 663, in execute
  File "D:\a\_work\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\azure/cli/core/commands/__init__.py", line 726, in _run_jobs_serially
  File "D:\a\_work\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\azure/cli/core/commands/__init__.py", line 718, in _run_job
  File "C:\Users\charris\.azure\cliextensions\containerapp\azext_containerapp\_client_factory.py", line 28, in _polish_bad_errors
    raise ex
  File "D:\a\_work\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\azure/cli/core/commands/__init__.py", line 697, in _run_job
  File "D:\a\_work\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\azure/cli/core/commands/__init__.py", line 333, in __call__
  File "D:\a\_work\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\azure/cli/core/commands/command_operation.py", line 121, in handler
  File "C:\Users\charris\.azure\cliextensions\containerapp\azext_containerapp\custom.py", line 627, in create_containerapp
    service_connectors_def_list, service_bindings_def_list = parse_service_bindings(cmd, service_bindings,
  File "C:\Users\charris\.azure\cliextensions\containerapp\azext_containerapp\_utils.py", line 542, in parse_service_bindings
    raise InvalidArgumentValueError("The Binding Name can only contain letters, numbers (0-9), periods ('.'), "
azure.cli.core.azclierror.InvalidArgumentValueError: The Binding Name can only contain letters, numbers (0-9), periods ('.'), and underscores ('_'). The length must not be more than 60 characters.

cli.azure.cli.core.azclierror: The Binding Name can only contain letters, numbers (0-9), periods ('.'), and underscores ('_'). The length must not be more than 60 characters.
az_command_data_logger: The Binding Name can only contain letters, numbers (0-9), periods ('.'), and underscores ('_'). The length must not be more than 60 characters.
cli.knack.cli: Event: Cli.PostExecute [<function AzCliLogging.deinit_cmd_metadata_logging at 0x048C0580>]


This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify src/index.json.

@azure-client-tools-bot-prd
Copy link

azure-client-tools-bot-prd bot commented Jul 19, 2023

️✔️Azure CLI Extensions Breaking Change Test
️✔️Non Breaking Changes

@azure-client-tools-bot-prd
Copy link

Hi @navba-MSFT,
Please write the description of changes which can be perceived by customers into HISTORY.rst.
If you want to release a new extension version, please update the version in setup.py as well.

@yonzhan
Copy link
Collaborator

yonzhan commented Jul 19, 2023

AzureContainerApp

@zhoxing-ms
Copy link
Contributor

@Greedygre Could you please help review this PR?

@@ -467,7 +467,7 @@ def process_service(cmd, resource_list, service_name, arg_dict, subscription_id,


def validate_binding_name(binding_name):
pattern = r'^(?=.{1,60}$)[a-zA-Z0-9._]+$'
pattern = r'^(?=.{1,60}$)[a-zA-Z0-9._-]+$'
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a message for this validate_binding_name, I'm not sure this binding name is available if it contains -

raise InvalidArgumentValueError("The Binding Name can only contain letters, numbers (0-9), periods ('.'), "
                                            "and underscores ('_'). The length must not be more than 60 characters.")

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @yash-nisar Do you know whether the binding name can contains - ?

@Greedygre
Copy link
Contributor

Greedygre commented Jul 20, 2023

Hi @yash-nisar
Could you please help to review this PR?
Do you know whether the binding name can contains - ?

@navba-MSFT
Copy link
Contributor Author

@yash-nisar @Greedygre Please let me know if you had a chance to look into this.

@yash-nisar
Copy link
Contributor

yash-nisar commented Jul 24, 2023

Currently, while using --bind, we expect the user to provide us the svc_name:[binding_name] where the binding_name is optional and the binding_name should follow

"The Binding Name can only contain letters, numbers (0-9), periods ('.'), and underscores ('_'). The length must not be more than 60 characters." because this is what service linker expects.

If we have a svc_name that has a "-", we use the same as our binding_name and ask the user to provide an acceptable binding_name. Since binding_names would appear in env vars, we cannot allow "-" in the binding_name plus service linker doesn't accept it.

IMO, this PR will break stuff and a more longer term solution should be pondered upon:

  1. Restrict the dev service names to the acceptable set. OR
  2. Map bad chars like "-" to an "_" OR generate an error asking for a compliant binding_name.

CC @duglin

@yash-nisar
Copy link
Contributor

There were talks going on about how to handle it and there is an issue on the same: https://github.com/orgs/serverless-paas-balam/projects/2/views/1?pane=issue&itemId=34057347

@charris-msft
Copy link

From a DevEx perspective, customers already face significant challenges naming azure resources. Some allow non-alphanumeric chars and some don't some allow uppercase, and some don't. It's the wild west out there.

The current model, allowing them to give something a name that will prevent future functionality is obviously broken. Ideally, I would like to see "Azure" support the same naming conventions on every service, so customers don't have to have special knowledge of a resource in order to provide a name for it. But I fear that would lead us to only allowing lowercase, alphanumeric chars which are harder for humans to read.

I'm also concerned about the idea of "restricting dev service names", but perhaps it'd due to a lack of understanding on my part. I need to better understand what exactly a "dev service" refers to because it seems to me what I'm passing to --bind is just the name of a container app.

I do like the idea of supporting a compliant binding name that differs from the container app name, and I believe most developers wouldn't have a problem with swapping the invalid "-" char for "_". But I would also give them the opportunity to specify the binding name themself - perhaps with an optional --bindng_name param that allows them to be explicit if needed.

@duglin
Copy link

duglin commented Jul 25, 2023 via email

@charris-msft
Copy link

charris-msft commented Jul 25, 2023

ok, I feel a bit foolish for not figuring that out, but in my feeble defense it doesn't just leap off the page (see screenshot below).

Perhaps a simple fix, while the team considers other options, is to just update the error message to let the user know they can specify their own binding name:

from this:

The Binding Name can only contain letters, numbers (0-9), periods ('.'), and underscores ('_'). The length must not be more than 60 characters.

to this:

The Binding Name can only contain letters, numbers (0-9), periods ('.'), and underscores ('_'). The length must not be more than 60 characters. 
By default, the binding name is the same as the service name you specified [my-aca-pgaddon], but you can override the default and specify your own compliant binding name like this --bind my-aca-pgaddon[:my_aca_pgaddon]

image

@navba-MSFT
Copy link
Contributor Author

I will close this PR for now and created the below PR to address this ask.
#6566

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Assign Auto assign by bot ContainerApp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

az containerapp create --bind "my-app" throws error when service has dashes in the name "-"
7 participants