Azure Container Apps session execute new api-version #8249

yitaopan · 2024-11-08T03:17:15Z

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

Related command

az containerapp session code-interpreter

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? (pip install wheel==0.30.0 required)
My extension version conforms to the Extension version schema

For new extensions:

My extension description/summary conforms to the Extension Summary Guidelines.

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 · 2024-11-08T03:17:17Z

⚠️Azure CLI Extensions Breaking Change Test

⚠️containerapp

rule cmd_name rule_message suggest_message

⚠️ 1006 - ParaAdd containerapp session code-interpreter delete-file cmd containerapp session code-interpreter delete-file added parameter path

⚠️ 1006 - ParaAdd containerapp session code-interpreter show-file-content cmd containerapp session code-interpreter show-file-content added parameter path

⚠️ 1006 - ParaAdd containerapp session code-interpreter show-file-metadata cmd containerapp session code-interpreter show-file-metadata added parameter path

⚠️ 1006 - ParaAdd containerapp session code-interpreter upload-file cmd containerapp session code-interpreter upload-file added parameter path

azure-client-tools-bot-prd · 2024-11-08T03:17:20Z

Hi @yitaopan,
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 · 2024-11-08T03:17:30Z

Thank you for your contribution! We will review the pull request and get back to you soon.

github-actions · 2024-11-08T03:18:21Z

CodeGen Tools Feedback Collection

Thank you for using our CodeGen tool. We value your feedback, and we would like to know how we can improve our product. Please take a few minutes to fill our codegen survey

github-actions · 2024-11-08T03:21:13Z

Hi @yitaopan

Release Suggestions

Module: containerapp

Update VERSION to 1.0.0b5 in src/containerapp/setup.py

Notes

For more info about extension versioning, please refer to Extension version schema

Greedygre · 2024-11-11T06:42:07Z

src/containerapp/azext_containerapp/_clients.py

@@ -1018,7 +1017,7 @@ def list(cls, cmd, resource_group_name, environment_name):


 class SessionPoolPreviewClient():
-    api_version = SESSION_API_VERSION
+    api_version = PREVIEW_API_VERSION


I will ask azure cli folks help to merge this PR: #8223

It will have some conflict with your PR.

This PR has been merge. Please resolve the conflict.

Merged and conflicts resolved.

Greedygre · 2024-11-11T06:47:47Z

src/containerapp/azext_containerapp/_clients.py

        request_url = url_fmt.format(
            session_pool_endpoint,
+            "path=" + path + "&" if path is not None else "",


Suggested change

"path=" + path + "&" if path is not None else "",

f"path={path}&" if path is not None else "",

src/containerapp/azext_containerapp/_clients.py

-    def show_file_content(cls, cmd, identifier, filename, session_pool_endpoint):
-        url_fmt = "{}/files/content/{}?identifier={}&api-version={}"
+    def show_file_content(cls, cmd, identifier, filename, path, session_pool_endpoint):
+        url_fmt = "{}/files/{}/content?{}identifier={}&api-version={}"


Greedygre · 2024-11-11T07:02:25Z

src/containerapp/azext_containerapp/_clients.py

        request_url = url_fmt.format(
            session_pool_endpoint,
            filename,
+            "path=" + path + "&" if path is not None else "",


Suggested change

"path=" + path + "&" if path is not None else "",

f"path={path}&" if path is not None else "",

Greedygre · 2024-11-11T07:02:31Z

src/containerapp/azext_containerapp/_clients.py

        request_url = url_fmt.format(
            session_pool_endpoint,
            filename,
+            "path=" + path + "&" if path is not None else "",


Suggested change

"path=" + path + "&" if path is not None else "",

f"path={path}&" if path is not None else "",

Greedygre · 2024-11-11T07:34:37Z

src/containerapp/azext_containerapp/_params.py

@@ -435,7 +435,7 @@ def load_arguments(self, _):
    with self.argument_context('containerapp session code-interpreter', arg_group='file') as c:
        c.argument('filename', help="The file to delete or show from the session")
        c.argument('filepath', help="The local path to the file to upload to the session")
-        c.argument('path', help="The path to list files from the session")
+        c.argument('path', help="The path of files in the session")


Recommend to add an example for --path in the _help.py

Greedygre · 2024-11-11T07:42:24Z

src/containerapp/azext_containerapp/containerapp_session_code_interpreter_decorator.py

        if self.get_argument_timeout_in_seconds() is not None:
-            self.session_code_interpreter_def["properties"]["timeoutInSeconds"] = self.get_argument_timeout_in_seconds()
+            self.session_code_interpreter_def["timeoutInSeconds"] = self.get_argument_timeout_in_seconds()
        else:
-            self.session_code_interpreter_def["properties"]["timeoutInSeconds"] = 60
-        self.session_code_interpreter_def["properties"]["code"] = self.get_argument_code()
+            self.session_code_interpreter_def["timeoutInSeconds"] = 60


The self.get_argument_timeout_in_seconds() will never be None because there is a default value:

--timeout-in-seconds : Duration in seconds code in session can run prior to timing out 0 - 60 secs, e.g. 30. Default: 60.

Make sense, fixed.

ShichaoQiu · 2024-11-11T07:45:37Z

src/containerapp/azext_containerapp/_clients.py

        request_url = url_fmt.format(
            session_pool_endpoint,
            filename,
+            "path=" + path + "&" if path is not None else "",


same as L1186

Thanks, fixed

Greedygre · 2024-11-11T08:02:54Z

src/containerapp/HISTORY.rst

@@ -7,6 +7,8 @@ upcoming
 * 'az containerapp create': Fix Role assignment error when the default Azure Container Registry could not be found
 * Upgrade api-version to 2024-10-02-preview
 * 'az containerapp create/update': `--yaml` support property pollingInterval and cooldownPeriod
+* 'az containerapp session code-interpreter': `--path` support upload-file/list-files/show-file-content/show-file-metadata/delete-file for code interpreter sessions


Suggested change

* 'az containerapp session code-interpreter': `--path` support upload-file/list-files/show-file-content/show-file-metadata/delete-file for code interpreter sessions

* 'az containerapp session code-interpreter upload-file/list-files/show-file-content/show-file-metadata/delete-file': Support `--path` to specify the path of code interpreter session

Juliehzl · 2024-11-11T09:13:09Z

src/containerapp/azext_containerapp/_models.py

-        "code": None,
-        "timeoutInSeconds": None
-    }
+    "codeInputType": None,


with one property identifier removed?

The backend Api is switched to use 2024-10-02-preview in this PR, in which the identifier property is removed in the payload, but still exists in the query parameter.

Juliehzl · 2024-11-11T09:14:24Z

...nerapp/azext_containerapp/tests/latest/test_containerapp_session_code_interpreter_nodelts.py

@@ -119,10 +109,5 @@ def test_containerapp_session_code_interpreter_nodelts_e2e(self, resource_group)
            resource_group,
            ))

-        self.cmd("containerapp sessionpool delete -n {} -g {} --yes".format(


why remove the deletion code?

This is to remove the deletion of that earlier created PythonLTS session pool https://github.com/Azure/azure-cli-extensions/pull/8249/files/dd6dc3deae0818beb7408a423208e136fd4d31a2#r1836222120

Juliehzl · 2024-11-11T09:15:15Z

...nerapp/azext_containerapp/tests/latest/test_containerapp_session_code_interpreter_nodelts.py

@@ -96,7 +86,7 @@ def test_containerapp_session_code_interpreter_nodelts_e2e(self, resource_group)
            identifier_name,
            "cert.txt"),
            checks=[
-            JMESPathCheck('properties.filename', 'cert.txt'),
+            JMESPathCheck('name', 'cert.txt'),


such change is breaking for cli users. Can we make sure there is no change for cli command output?

There are breaking changes in the output of these commands in api-version 2024-10-02-preview, which is reviewed by swagger and sdk team.
The HISTORY.rst file is updated for this.

Even there is breaking change in swagger, we should still avoid breaking change in cli commands. Discussed with hong, given there is no cli usage, we could allow cli breaking change this time. But in the future, with increasing cli users, such breaking change is not allowed.

Thank you, understood, will keep in mind for this in the future.

Juliehzl · 2024-11-11T09:15:38Z

...nerapp/azext_containerapp/tests/latest/test_containerapp_session_code_interpreter_nodelts.py

@@ -25,16 +25,6 @@ def test_containerapp_session_code_interpreter_nodelts_e2e(self, resource_group)
        sessionpool_list = self.cmd("containerapp sessionpool list -g {}".format(resource_group)).get_output_in_json()
        self.assertTrue(len(sessionpool_list) == 0)

-        # Create PythonLTS SessionPool with default container type which is PythonLTS
-        sessionpool_name_pythonlts = self.create_random_name(prefix='sppythonlts', length=24)
-        self.cmd('containerapp sessionpool create -g {} -n {} --cooldown-period {}'.format(


why remove this command?

Because I think this file should be focusing on NodeLTS session pool, and all the validation is targeting NodeLTS pool rather than the Python session pool created here.
And for the tests and validations against PythonLTS session pools are duplicated in another test file test_containerapp_session_code_interpreter.py.

Juliehzl · 2024-11-11T09:16:17Z

...nerapp/azext_containerapp/tests/latest/test_containerapp_session_code_interpreter_nodelts.py

+            JMESPathCheck('status', 'Succeeded'),
+            JMESPathCheck('result.stdout', 'Hello world\n')


avoid such breaking change. recommend to transform output for consistence

Greedygre · 2024-11-12T02:25:31Z

src/containerapp/azext_containerapp/_clients.py

+    @classmethod
+    def extract_path_from_filename(cls, path, filename):
+        if '/' not in filename:
+            return path, filename
+        path_in_filename, filename = filename.rsplit('/', 1)
+        if path is None:
+            return path_in_filename, filename
+        else:
+            return path.rstrip("/") + "/" + path_in_filename.lstrip('/'), filename


this should be a static method.

Suggested change

@classmethod

def extract_path_from_filename(cls, path, filename):

if '/' not in filename:

return path, filename

path_in_filename, filename = filename.rsplit('/', 1)

if path is None:

return path_in_filename, filename

else:

return path.rstrip("/") + "/" + path_in_filename.lstrip('/'), filename

@staticmethod

def extract_path_from_filename(path, filename):

if '/' not in filename:

return path, filename

path_in_filename, filename = filename.rsplit('/', 1)

if path is None:

return path_in_filename, filename

full_path = f"{path.rstrip('/')}/{path_in_filename.lstrip('/')}"

return full_path, filename

Thank you, fixed

Greedygre · 2024-11-12T05:13:27Z

Hi @yonzhan @zhoxing-ms
This PR has been reviewed and approved inner our team, can you help to review it when you have chance? Thanks!

yanzhudd · 2024-11-12T08:10:03Z

/azp run

azure-pipelines · 2024-11-12T08:10:16Z

Azure Pipelines successfully started running 2 pipeline(s).

Greedygre · 2024-11-12T08:27:36Z

src/containerapp/azext_containerapp/_clients.py

@@ -1281,6 +1287,16 @@ def list_files(cls, cmd, identifier, path, session_pool_endpoint):

        r = send_raw_request(cmd.cli_ctx, "GET", request_url, resource=SESSION_RESOURCE)
        return r.json()
+


CI style failed
src/containerapp/azext_containerapp/_clients.py:1290:0: C0303: Trailing whitespace (trailing-whitespace)

remove the whitespece

Thanks, fixed

yanzhudd · 2024-11-12T08:45:07Z

/azp run

azure-pipelines · 2024-11-12T08:45:20Z

Azure Pipelines successfully started running 2 pipeline(s).

yitaopan added 2 commits November 7, 2024 18:32

Existing commands switched to new api

8fa3228

Update api-version

c12450a

azure-client-tools-bot-prd bot added this to the November 2024 (2024-11-19)❗ Breaking change release ❗ milestone Nov 8, 2024

microsoft-github-policy-service bot requested review from yonzhan, wangzelin007, yanzhudd, zhoxing-ms, jsntcy, ruslany, sanchitmehta, ebencarek, JennyLawrance, howang-ms, vinisoto, chinadragon0515, vturecek, torosent, pagariyaalok, Juliehzl, jijohn14 and Greedygre November 8, 2024 03:17

microsoft-github-policy-service bot added the Auto-Assign Auto assign by bot label Nov 8, 2024

microsoft-github-policy-service bot assigned zhoxing-ms Nov 8, 2024

microsoft-github-policy-service bot added the ContainerApp label Nov 8, 2024

File back to filepath

9a5113e

Greedygre reviewed Nov 11, 2024

View reviewed changes

ShichaoQiu reviewed Nov 11, 2024

View reviewed changes

Greedygre reviewed Nov 11, 2024

View reviewed changes

Fix

cb122e6

Juliehzl reviewed Nov 11, 2024

View reviewed changes

Merge main, resolve conflicts, rerun tests

be3d72d

Greedygre reviewed Nov 12, 2024

View reviewed changes

Fix staticmethod and helper doc

6b65c01

Juliehzl approved these changes Nov 12, 2024

View reviewed changes

Greedygre approved these changes Nov 12, 2024

View reviewed changes

Merge main, resolve conflicts

71ca21c

yanzhudd approved these changes Nov 12, 2024

View reviewed changes

Greedygre reviewed Nov 12, 2024

View reviewed changes

Fix style

18bf7af

yanzhudd merged commit cf2c792 into Azure:main Nov 12, 2024
21 checks passed

	"path=" + path + "&" if path is not None else "",
	f"path={path}&" if path is not None else "",

	* 'az containerapp session code-interpreter': `--path` support upload-file/list-files/show-file-content/show-file-metadata/delete-file for code interpreter sessions
	* 'az containerapp session code-interpreter upload-file/list-files/show-file-content/show-file-metadata/delete-file': Support `--path` to specify the path of code interpreter session

		JMESPathCheck('status', 'Succeeded'),
		JMESPathCheck('result.stdout', 'Hello world\n')

		@@ -1281,6 +1287,16 @@ def list_files(cls, cmd, identifier, path, session_pool_endpoint):

		r = send_raw_request(cmd.cli_ctx, "GET", request_url, resource=SESSION_RESOURCE)
		return r.json()

Azure Container Apps session execute new api-version #8249

Azure Container Apps session execute new api-version #8249

Conversation

yitaopan commented Nov 8, 2024 • edited Loading

Related command

General Guidelines

About Extension Publish

azure-client-tools-bot-prd bot commented Nov 8, 2024 • edited Loading

azure-client-tools-bot-prd bot commented Nov 8, 2024

yonzhan commented Nov 8, 2024

github-actions bot commented Nov 8, 2024

CodeGen Tools Feedback Collection

github-actions bot commented Nov 8, 2024 • edited Loading

Release Suggestions

Module: containerapp

Notes

Greedygre Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitaopan Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitaopan Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Greedygre commented Nov 12, 2024

yanzhudd commented Nov 12, 2024

azure-pipelines bot commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanzhudd commented Nov 12, 2024

azure-pipelines bot commented Nov 12, 2024

yitaopan commented Nov 8, 2024 •

edited

Loading

azure-client-tools-bot-prd bot commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 8, 2024 •

edited

Loading

Greedygre Nov 11, 2024 •

edited

Loading

yitaopan Nov 11, 2024 •

edited

Loading

yitaopan Nov 11, 2024 •

edited

Loading