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

[ContainerApp] Fix #501: az containerapp create --allow-insecure: Add Flag to Allow Container App Insecure Ingress #6555

Merged
merged 13 commits into from
Aug 9, 2023

Conversation

Juancpani
Copy link
Contributor

@Juancpani Juancpani commented Jul 25, 2023


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 25, 2023

⚠️Azure CLI Extensions Breaking Change Test
⚠️containerapp
rule cmd_name rule_message suggest_message
⚠️ 1006 - ParaAdd containerapp create cmd containerapp create added parameter allow_insecure

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

Hi @Juancpani,
Since the current milestone time is less than 7 days, this pr will be reviewed in the next milestone.

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

Hi @Juancpani,
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 25, 2023

ContainerApp

Copy link
Member

@smzeng smzeng left a comment

Choose a reason for hiding this comment

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

add a test? Not having a test might block you from getting merged. Or maybe just edit a current one since its a small change

Copy link
Member

@smzeng smzeng left a comment

Choose a reason for hiding this comment

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

is this just for create, we aren't adding it to update?

Copy link
Member

@smzeng smzeng left a comment

Choose a reason for hiding this comment

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

also update the src/containerapp/HISTORY.rst file with this change just for book keeping

@Juancpani Juancpani closed this Jul 28, 2023
@Juancpani Juancpani reopened this Jul 31, 2023
@Juancpani Juancpani closed this Jul 31, 2023
@Juancpani Juancpani reopened this Aug 3, 2023
@Juancpani
Copy link
Contributor Author

is this just for create, we aren't adding it to update?

update already has it, just create was missing it.

@Juancpani Juancpani requested a review from smzeng August 3, 2023 16:40
@cormacpayne
Copy link
Member

is this just for create, we aren't adding it to update?

update already has it, just create was missing it.

@Juancpani I see the --allow-insecure flag for the az containerapp ingress update command but not for az containerapp update -- just wanted to double-check if that's what we were expecting, or if it needed to be added to the latter command.

@Juancpani
Copy link
Contributor Author

Juancpani commented Aug 3, 2023

is this just for create, we aren't adding it to update?

update already has it, just create was missing it.

@Juancpani I see the --allow-insecure flag for the az containerapp ingress update command but not for az containerapp update -- just wanted to double-check if that's what we were expecting, or if it needed to be added to the latter command.

Just realized my mistake, custom.py has update_ingress which functions as the ingress update function, but no containerapp update isn't used to update any of the previous ingress settings, so I doubt it will need this flag.

Copy link
Member

@smzeng smzeng left a comment

Choose a reason for hiding this comment

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

Not sure you did this already, but you can setup a storage account and upload a build of your cli environment as a whl using
azdev extension publish containerapp --storage-account STORAGE_ACCOUNT --storage-container STORAGE_CONTAINER --storage-account-key STORAGE_KEY

then anyone can add that version of the cli with az extension add --upgrade --source LINKTOWHL

If you do that, leave a comment and then I can also help test out the changes that way. Might be easier for us to catch bugs thats way too that we miss from just reading the changes

@Juancpani Juancpani reopened this Aug 8, 2023
@Juancpani
Copy link
Contributor Author

@yonzhan this is ready to go!

@zhoxing-ms zhoxing-ms merged commit bb4832c into Azure:main Aug 9, 2023
14 checks passed
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.

6 participants