Containerapp 0.3.8 Release by StrawnSC · Pull Request #5026 · Azure/azure-cli-extensions

StrawnSC · 2022-06-21T20:32:30Z

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:

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.
The precondition is to put your code inside this repository and upgrade the version in the pull request but do not modify src/index.json.

yonzhan · 2022-06-22T00:02:06Z

Containerapp

* init containerapp version 0.3.7 * Use PATCH API for containerapp update. * Removed double lines. * fix bug * Fix microsoft/azure-container-apps#261 * Fixed issue where containerapp up would not honor --registry-server. (#128) * Fixed issue where containerapp up would not honor --registry-server. * Updated history. Co-authored-by: Haroon Feisal <haroonfeisal@microsoft.com> * bug fix * add tests * fix credscan suppressions * Use constant for acr registry prefix. * Fixed multi-container issue. * Removed comment. * Updated update_containerapp_yaml to use patch. Fixed minor style errors. * Fixed merge conflicts. * Rerecorded necessary tests. * Updated history. * Auto-populate secret values if passed without values. * Fixed bug where the containerapp had no secrets. Co-authored-by: Silas Strawn <strawnsc@gmail.com> Co-authored-by: Haroon Feisal <haroonfeisal@microsoft.com>

…to ci suppressions. Reran tests. (#135) Co-authored-by: Haroon Feisal <haroonfeisal@microsoft.com>

* Fixed yaml breaking change and bug where revision suffix would return invalid if user is adding a container and the previous revision has a revision suffix. * Fixed revision suffix yaml bug. Co-authored-by: Haroon Feisal <haroonfeisal@microsoft.com>

Validate containerapp name.

Fix log stream bug

panchagnula · 2022-07-20T16:06:06Z

@zhoxing-ms when you are online today could you review this please, we would like to get this payload released soon. Thanks!

scripts/ci/credscan/CredScanSuppressions.json

src/containerapp/HISTORY.rst

src/containerapp/azext_containerapp/_up_utils.py

src/containerapp/azext_containerapp/_utils.py

panchagnula · 2022-07-20T16:29:49Z

Unrelated to changes on this PR , however, want to understand this code.
what is the purpose of this? Maintaining this means we keep track of any new read_only_attributes added & update this list?
def _remove_readonly_attributes(containerapp_def):
unneeded_properties = [
"id",
"name",
"type",
"systemData",
"provisioningState",
"latestRevisionName",
"latestRevisionFqdn",
"customDomainVerificationId",
"outboundIpAddresses",
"fqdn"
]

panchagnula

a few Qs/ comments in the chat. @haroonf / @StrawnSC please check. Thanks!

panchagnula · 2022-07-20T16:31:35Z

@calvinsID please review the changes as well, esp. the update using PATCH. Thanks!

StrawnSC · 2022-07-21T23:50:26Z

Unrelated to changes on this PR , however, want to understand this code.
what is the purpose of this? Maintaining this means we keep track of any new read_only_attributes added & update this list?
def _remove_readonly_attributes(containerapp_def):
unneeded_properties = [
"id",
"name",
"type",
"systemData",
"provisioningState",
"latestRevisionName",
"latestRevisionFqdn",
"customDomainVerificationId",
"outboundIpAddresses",
"fqdn"
]

@panchagnula looks like this is needed since we're not using the SDK. The SDK automatically ignores readonly attributes that are set on a model before serializing it. @calvinsID is that right? (I believe you authored that part if I'm not mistaken since it's in the yaml command)

calvinsID · 2022-07-22T04:30:26Z

Unrelated to changes on this PR , however, want to understand this code.
what is the purpose of this? Maintaining this means we keep track of any new read_only_attributes added & update this list?
def _remove_readonly_attributes(containerapp_def):
unneeded_properties = [
"id",
"name",
"type",
"systemData",
"provisioningState",
"latestRevisionName",
"latestRevisionFqdn",
"customDomainVerificationId",
"outboundIpAddresses",
"fqdn"
]

@panchagnula looks like this is needed since we're not using the SDK. The SDK automatically ignores readonly attributes that are set on a model before serializing it. @calvinsID is that right? (I believe you authored that part if I'm not mistaken since it's in the yaml command)

Yeah this is needed since not using SDK. These properties shouldn't be passed into any PUT/PATCH api, but these properties are introduced from deserialization where normally SDK handles it

panchagnula · 2022-07-22T15:42:51Z

src/containerapp/azext_containerapp/_up_utils.py

    if not is_valid:
-        raise ValidationError(f"Invalid Container App name {name}. A name must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character and cannot have '--'. The length must not be more than 32 characters.")
+        raise ValidationError(f"Invalid Container App name {name}. A name must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character and cannot have '--'. The length must not be more than {MAXIMUM_CONTAINER_APP_NAME_LENGTH} characters.")



seems like the message is copied from portal ;) but "alphabetic character" seems incorrect English to me.

This message is copied from the API

panchagnula · 2022-07-22T15:44:57Z

@StrawnSC - some of my comments are still not completely addressed - ex. adding comments to the helper library, explaining the need for those . & update help for container app name parameter, so that the format requirements are displayed in help docs?

panchagnula · 2022-07-22T16:44:37Z

src/containerapp/azext_containerapp/_utils.py

+    return d
+
+
+def _populate_secret_values(containerapp_def, secret_values):


@haroonf , don't need to change this logic for now, since we need to release the fix.
Can you help me understand the behavior in CLI with YAML here , when CLI parses the YAML with secretName & no value, does the payload to the API translate this object in JSON payload just name property & no value i.e{ name: xyz} or. {name: xyz, value: null}. if it is the former is the PATCH API, setting the value to null or is it cleaning these up because CLI explicitly passes null?

The deserialization process leaves the json payload to {name: xyz, value: null}. The API returns an error "cannot set secret values to null."

StrawnSC · 2022-07-22T16:48:16Z

@StrawnSC - some of my comments are still not completely addressed - ex. adding comments to the helper library, explaining the need for those . & update help for container app name parameter, so that the format requirements are displayed in help docs?

@panchagnula Sorry about that - I must have misread your comment on the help text. Just pushed those changes.

@haroonf can you take a look at Sisira's comment above

panchagnula

LGTM - please do update the comments if possible in this one. & answer the pending Qs

StrawnSC · 2022-07-25T19:01:58Z

@zhoxing-ms could you take a look at this when you get the chance? We've had some customers asking about these fixes for a while

start containerapp version 0.3.8

bfaaaf1

ghost requested a review from yonzhan June 21, 2022 20:32

ghost added the Auto-Assign Auto assign by bot label Jun 21, 2022

ghost requested a review from wangzelin007 June 21, 2022 20:32

ghost assigned zhoxing-ms Jun 21, 2022

ghost added this to the Jun 2022 (2022-07-05) milestone Jun 21, 2022

ghost added the ContainerApp label Jun 21, 2022

ghost requested review from jsntcy and zhoxing-ms June 21, 2022 20:32

continue logstream/ssh even if pinging the app fails

5f3b965

StrawnSC mentioned this pull request Jul 11, 2022

Use PATCH API for containerapp update. calvinsID/azure-cli-extensions#129

Merged

3 tasks

runefa and others added 8 commits July 15, 2022 12:59

Fixed bug. Renamed test with existing name. Added new test recording …

9f28b52

…to ci suppressions. Reran tests. (#135) Co-authored-by: Haroon Feisal <haroonfeisal@microsoft.com>

rerecord test

35fddc1

Validate containerapp name.

8cce1f9

Merge pull request #137 from haroonf/upnamevalidation

284bd3d

Validate containerapp name.

Merge pull request #134 from StrawnSC/fix-logstream

53e83b0

Fix log stream bug

rerecord test

d8ceed7

StrawnSC marked this pull request as ready for review July 19, 2022 16:50

StrawnSC requested review from calvinsID, panchagnula and runefa as code owners July 19, 2022 16:50

Update HISTORY.rst

6824d16