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.

Sign up for GitHub

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

Jump to bottom

Containerapp 0.3.8 Release #5026

Merged
merged 13 commits into from
Jul 26, 2022
Merged

Containerapp 0.3.8 Release #5026

merged 13 commits into from
Jul 26, 2022

Conversation

StrawnSC
Copy link
Contributor

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.
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.

@ghost ghost requested a review from yonzhan June 21, 2022 20:32
@ghost ghost added the Auto-Assign Auto assign by bot label Jun 21, 2022
@ghost ghost requested a review from wangzelin007 June 21, 2022 20:32
@ghost ghost assigned zhoxing-ms Jun 21, 2022
@ghost ghost added this to the Jun 2022 (2022-07-05) milestone Jun 21, 2022
@ghost ghost added the ContainerApp label Jun 21, 2022
@ghost ghost requested review from zhoxing-ms and jsntcy June 21, 2022 20:32
@yonzhan
Copy link
Collaborator

yonzhan commented Jun 22, 2022

Containerapp

@StrawnSC StrawnSC mentioned this pull request Jul 11, 2022
Merged
3 tasks
runefa and others added 8 commits July 15, 2022 12:59
@runefa @StrawnSC
Use PATCH API for containerapp update. (#129)
4995049 
* 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 <[email protected]>

* 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 <[email protected]>
Co-authored-by: Haroon Feisal <[email protected]>
@runefa
Fixed bug. Renamed test with existing name. Added new test recording …
9f28b52 
…to ci suppressions. Reran tests. (#135)

Co-authored-by: Haroon Feisal <[email protected]>
@StrawnSC
rerecord test
35fddc1
@runefa
Fixed update yaml bug with revision suffix (#136)
e55116f 
* 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 <[email protected]>
Validate containerapp name.
8cce1f9
@StrawnSC
Merge pull request #137 from haroonf/upnamevalidation
284bd3d 
Validate containerapp name.
@StrawnSC
Merge pull request #134 from StrawnSC/fix-logstream
53e83b0 
Fix log stream bug
@StrawnSC
rerecord test
d8ceed7
@StrawnSC StrawnSC marked this pull request as ready for review July 19, 2022 16:50
@panchagnula
Copy link
Contributor

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

@panchagnula
Copy link
Contributor

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
panchagnula suggested changes Jul 20, 2022
View reviewed changes
Copy link
Contributor

@panchagnula panchagnula left a comment

Choose a reason for hiding this comment

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

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

@panchagnula
Copy link
Contributor

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

@StrawnSC
Copy link
Contributor Author

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
Copy link
Contributor

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
panchagnula reviewed Jul 22, 2022
View reviewed changes
src/containerapp/azext_containerapp/_up_utils.py
is_valid = is_valid and '--' not in name
name = name.replace('-', '')
is_valid = is_valid and name.isalnum()
is_valid = is_valid and name[0].isalpha()
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.")

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This message is copied from the API

@panchagnula
Copy link
Contributor

@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
panchagnula reviewed Jul 22, 2022
View reviewed changes
src/containerapp/azext_containerapp/_utils.py
return d


def _populate_secret_values(containerapp_def, secret_values):
Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@StrawnSC
Copy link
Contributor Author

@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
panchagnula approved these changes Jul 22, 2022
View reviewed changes
Copy link
Contributor

@panchagnula panchagnula left a comment

Choose a reason for hiding this comment

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

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

@StrawnSC
Copy link
Contributor Author

@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

@zhoxing-ms zhoxing-ms merged commit 8165036 into Azure:main Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhoxing-ms zhoxing-ms zhoxing-ms approved these changes

@calvinsID calvinsID calvinsID approved these changes

@panchagnula panchagnula panchagnula approved these changes

@yonzhan yonzhan Awaiting requested review from yonzhan

@wangzelin007 wangzelin007 Awaiting requested review from wangzelin007

@jsntcy jsntcy Awaiting requested review from jsntcy

@runefa runefa Awaiting requested review from runefa

Assignees

@zhoxing-ms zhoxing-ms

Labels
Auto-Assign Auto assign by bot ContainerApp
Projects
None yet
Milestone
Jun 2022 (2022-07-05)
Development

Successfully merging this pull request may close these issues.
6 participants
@StrawnSC @yonzhan @panchagnula @calvinsID @zhoxing-ms @runefa