-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add terminationGracePeriodSeconds support For container app #6439
Add terminationGracePeriodSeconds support For container app #6439
Conversation
Hi @njuCZ, |
Thank you for your contribution! We will review the pull request and get back to you soon. |
78ed7f3
to
5a6c69d
Compare
@@ -120,6 +120,7 @@ def load_arguments(self, _): | |||
c.argument('traffic_weights', nargs='*', options_list=['--traffic-weight'], help="A list of revision weight(s) for the container app. Space-separated values in 'revision_name=weight' format. For latest revision, use 'latest=weight'") | |||
c.argument('workload_profile_name', options_list=['--workload-profile-name', '-w'], help="Name of the workload profile to run the app on.", is_preview=True) | |||
c.argument('secret_volume_mount', help="Path to mount all secrets e.g. mnt/secrets", is_preview=True) | |||
c.argument('termination_grace_period', type=int, options_list=['--termination-grace-period', '--grace-period'], help="Duration in seconds a replica is given to gracefully shut down before it is forcefully terminated. (Default: 30)", is_preview=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it supported in --yaml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The termination_grace_period already existed in _sdk_models, so it is supported in --yaml.
@njuCZ Could you please add some test for property termination_grace_period in test_containerapp_create_with_yaml or add new test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only api-version >= 2023-04-01-preview support this property. we are still using python sdk 2022-11-01-preview, so it's not supported in --yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can manual add this property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
d235e5f
to
8a58213
Compare
Hi @zhoxing-ms |
8a58213
to
a74c5b3
Compare
src/containerapp/HISTORY.rst
Outdated
@@ -17,6 +17,7 @@ Upcoming | |||
* Add 'az containerapp ingress cors' for CORS support | |||
* 'az container app env create/update': support --enable-mtls parameter | |||
* 'az containerapp up': fix issue where --repo throws KeyError | |||
* 'az containerapp create/update': --termination-grace-period support custom termination grace period |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not add the history note into an extension version which had already been released. Please take a look at this comment #6439 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this means support a new property when az containerapp create/update, do you think I need to make the description better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to release a new extension version for this PR, you need to upgrade the version defined in HISTORY.rst
and setup.py
.
If you don't want to release a new extension version, you can put the history note under Upcoming
in HISTORY.rst
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebase main branch to fix a conflict just now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems US team released a new version last night... Have updated it
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
General Guidelines
azdev style <YOUR_EXT>
locally? (pip install azdev
required)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
.