containerapp - Add --source and --repo to containerapp create #6463

snehapar9 · 2023-06-30T15:54:11Z

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.
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 · 2023-06-30T15:54:13Z

Hi @snehapar9,
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.

azure-client-tools-bot-prd · 2023-06-30T15:54:16Z

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

yonzhan · 2023-06-30T16:00:30Z

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

cormacpayne

A few initial comments to take a look at

cormacpayne · 2023-06-30T17:58:03Z

src/containerapp/azext_containerapp/_help.py

              --image my-app:v1.0 --environment MyContainerappEnv \\
              --secrets mysecret=secretvalue1 anothersecret="secret value 2" \\
              --secret-volume-mount "mnt/secrets"
+    - name: Create a container app from a dockerfile in a GitHub repo (setting up github actions)


nit: "github actions" --> "GitHub Actions"

cormacpayne · 2023-06-30T18:16:33Z

src/containerapp/azext_containerapp/_help.py

              --image my-app:v1.0 --environment MyContainerappEnv \\
              --secrets mysecret=secretvalue1 anothersecret="secret value 2" \\
              --secret-volume-mount "mnt/secrets"
+    - name: Create a container app from a dockerfile in a GitHub repo (setting up github actions)


Let's remove the "Dockerfile" part of this since our GitHub Action doesn't require the application source to have one. It may be worth thinking about rephrasing this snippet as follows:

"Create a Container App from a new GitHub Actions workflow in the provided repository."

cormacpayne · 2023-06-30T18:19:25Z

src/containerapp/azext_containerapp/_help.py

+          --environment MyContainerappEnv --registry-server MyRegistryServer \\
+          --registry-user MyRegistryUser --registry-pass MyRegistryPass \\
+          --repo https://github.com/myAccount/myRepo
+    - name: Create a container app from a dockerfile in a local directory (or autogenerate a container if no dockerfile is found)


Similar comment as above: I would recommend removing the mention of "Dockerfile" and think about rephrasing this snippet as follows:

"Create a Container App from the provided application source."

cormacpayne · 2023-06-30T18:20:33Z

src/containerapp/azext_containerapp/_up_utils.py

+    if source and repo:
+        raise MutuallyExclusiveArgumentError(
+            "Cannot use --source and --repo togther. "
+            "Can either deploy from a local directory or a Github repo"


nit: "Github repo" --> "GitHub repository"

cormacpayne · 2023-06-30T18:32:06Z

src/containerapp/azext_containerapp/custom.py

    if not image:
+        imageNotProvided = True
        image = HELLO_WORLD_IMAGE
+    token = get_token(cmd, repo, token)


nit: can we move this line down to where it's used first? Preferably around the if-statement on 716

cormacpayne · 2023-06-30T20:13:28Z

src/containerapp/azext_containerapp/custom.py

+          dockerfile_content = _get_dockerfile_content(repo, branch, token, source, context_path, dockerfile)
+          ingress, target_port = _get_ingress_and_target_port(ingress, target_port, dockerfile_content)
+
+        app.create_acr_if_needed()


Are we able to remove this? Even if it doesn't create the ACR instance in any of the flows provided by this function, if the implementation changes under-the-hood, it could start causing some unexpected behavior when we don't want the ACR instance to be created in any circumstance.

Removed this line. Since it is now required to set registry-server, registry-user and registry-pass args, the method create_acr_if_needed() is not longer required

cormacpayne · 2023-06-30T20:19:35Z

src/containerapp/azext_containerapp/custom.py

        else:
            set_managed_identity(cmd, resource_group_name, containerapp_def, user_assigned=[registry_identity])
    try:
+        image = None if imageNotProvided else _reformat_image(source,repo,image)


I'm not sure I understand the relationship between imageNotProvided and image if we set them to True and HELLO_WORLD_IMAGE, respectively, at the beginning of this function, but then revert image back to None if imageNotProvided is True. Would you mind elaborating on the purpose of this line?

Changed this line to image = None if self.get_argument_image().__eq__(HELLO_WORLD_IMAGE) else _reformat_image(self.get_argument_source(), self.get_argument_repo(), self.get_argument_image())

Since image is defaulted to mcr.microsoft.com/k8se/quickstart:latest if it was not provided by the user, we need to reassign it to None so the final image pushed to the registry is in the format <registry-server>/<containerapp-name>:tag

Greedygre · 2023-07-03T04:58:33Z

src/containerapp/azext_containerapp/custom.py

    validate_container_app_name(name, AppType.ContainerApp.name)
    validate_create(registry_identity, registry_pass, registry_user, registry_server, no_wait)
    validate_revision_suffix(revision_suffix)
+    _validate_create_args(cmd, source, repo, registry_server, registry_user, registry_pass)


Please rebase newest code and move following validate logic to ContainerAppCreateDecorator.validate_arguments

_validate_create_args(cmd, source, repo, registry_server, registry_user, registry_pass)

Merged latest changes from main

…to snehapar/Add-source-and-repo-to-create

cormacpayne

Some nits, but the core changes look good to me.

For testing:
(1) We should be able to replicate the --source test that we have for up with this new create flow
(2) Let's confirm that we've tested the --repo flow locally since we're not able to create a test for it (due to GitHub interactions)
(3) Let's confirm that the existing create flows continue to work as expected (either by re-running any create tests that exist today or running the flow locally)

cormacpayne · 2023-07-06T23:55:47Z

src/containerapp/azext_containerapp/_up_utils.py

                         LOG_ANALYTICS_RP,
                         CONTAINER_APPS_RP,
                         ACR_IMAGE_SUFFIX,
-                         MAXIMUM_CONTAINER_APP_NAME_LENGTH,


nit: intended change?

Yep, MAXIMUM_CONTAINER_APP_NAME_LENGTH is not used anywhere in this file.

cormacpayne · 2023-07-06T23:56:44Z

src/containerapp/azext_containerapp/_validators.py

-def validate_create(registry_identity, registry_pass, registry_user, registry_server, no_wait):
+def validate_create(registry_identity, registry_pass, registry_user, registry_server, no_wait, source=None, repo=None):
+    if source and repo:
+        raise MutuallyExclusiveArgumentError("Cannot use --source and --repo together. ""Can either deploy from a local directory or a Github repository")


nit: intended "" in the middle of the error message?

Great catch! Not intended.

cormacpayne · 2023-07-07T00:11:52Z

src/containerapp/azext_containerapp/containerapp_decorator.py

            registries_def["identity"] = self.get_argument_registry_identity()
            safe_set(containerapp_def, "properties", "configuration", "registries", value=[registries_def])
-            return containerapp_def
+        return containerapp_def


nit: stylistically this looks correct, but just want to ensure this change was intended

This change was not intended.

Juliehzl · 2023-07-10T07:21:48Z

src/containerapp/azext_containerapp/containerapp_decorator.py

            registries_def["identity"] = self.get_argument_registry_identity()
            safe_set(containerapp_def, "properties", "configuration", "registries", value=[registries_def])
-        return containerapp_def
+            return containerapp_def


with this change, there is no return out of if

Don't think it would matter functionality wise since we are always returning containerapp_def on line 586.(This is how it currently is in main. I agree stylistically, it is better to return out of the if statement instead.

Juliehzl · 2023-07-10T07:22:39Z

src/containerapp/azext_containerapp/tests/latest/test_containerapp_create.py

+TEST_DIR = os.path.abspath(os.path.join(os.path.abspath(__file__), '..'))
+
+class ContainerAppCreateTest(ScenarioTest):
+    @live_only()


avoid live_only if the tests are replayable

This test needs to be a live test because build packs requires docker to be running.
I'm not sure how to determine if the other tests can be replayed or not. Without live-only I had to first delete the yaml files to be able to re-run them successfully.

Juliehzl · 2023-07-10T07:26:07Z

src/containerapp/azext_containerapp/_params.py

        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', '--tgp'], help="Duration in seconds a replica is given to gracefully shut down before it is forcefully terminated. (Default: 30)", is_preview=True)
+        c.argument('source', help="Local directory path containing the application source and Dockerfile for building the container image. Preview: If no Dockerfile is present, a container image is generated using buildpacks. If Docker is not running or buildpacks cannot be used, Oryx will be used to generate the image. See the supported Oryx runtimes here: https://github.com/microsoft/Oryx/blob/main/doc/supportedRuntimeVersions.md.")


if the argument is in preview, add is_preview=True

These flags are not in preview.

Juliehzl · 2023-07-10T07:36:04Z

src/containerapp/azext_containerapp/containerapp_decorator.py

+            app = self.set_up_create_containerapp_if_source_or_repo(containerapp_def=containerapp_def)
+            containerapp_def = self.set_up_create_containerapp_source(app=app, containerapp_def=containerapp_def)


Why do you need two steps to construct container app payload? Is app required?

app is an instance of ContainerApp and is required in the set_up_create_containerapp_source method to properly set up source and repo for containerapp create (similar to containerapp up)

snehapar9 · 2023-07-10T21:46:47Z

Some nits, but the core changes look good to me.

For testing: (1) We should be able to replicate the --source test that we have for up with this new create flow (2) Let's confirm that we've tested the --repo flow locally since we're not able to create a test for it (due to GitHub interactions) (3) Let's confirm that the existing create flows continue to work as expected (either by re-running any create tests that exist today or running the flow locally)

Added integration tests for --source.
az containerapp create -g -n --environment --registry-server --registry-user registry-user --registry-pass registry-pass --repo GitHubRepoUrl created a workflow in the repo and ran successfully.
Not sure if there are pre-existing explicit tests for containerapp create - The az containerapp create command was run here
but locally, az containerapp create -g resource-group --environment --name < containerapp-name> ran as expected.

Greedygre · 2023-07-14T02:04:22Z

src/containerapp/azext_containerapp/containerapp_decorator.py

+            r = self.set_up_create_containerapp_repo(app=app, r=r, env=app.env, env_rg=app.resource_group.name)
+        return r
+
+    def set_up_create_containerapp_if_source_or_repo(self, containerapp_def):


Hi, I have refactored the containerapp decorator with this pr: containerapp refactor decorator by Greedygre · Pull Request #6511 · Azure/azure-cli-extensions (github.com)
You can use self.containerapp_def to set and get current payload for containerapp.
Thanks for using this.

Greedygre · 2023-07-14T02:05:44Z

src/containerapp/azext_containerapp/containerapp_decorator.py

+
+        # Update image
+        containerapp_def["properties"]["template"]["containers"][0]["image"] = HELLO_WORLD_IMAGE if app.image is None else app.image
+        return containerapp_def


with newest code, use self.containerapp_def to construct payload, no need to return.

azure-client-tools-bot-prd · 2023-07-25T19:19:30Z

⚠️Azure CLI Extensions Breaking Change Test

⚠️containerapp

rule cmd_name rule_message suggest_message

⚠️ 1006 - ParaAdd containerapp create cmd containerapp create added parameter branch

⚠️ 1006 - ParaAdd containerapp create cmd containerapp create added parameter context_path

⚠️ 1006 - ParaAdd containerapp create cmd containerapp create added parameter repo

⚠️ 1006 - ParaAdd containerapp create cmd containerapp create added parameter service_principal_client_id

⚠️ 1006 - ParaAdd containerapp create cmd containerapp create added parameter service_principal_client_secret

⚠️ 1006 - ParaAdd containerapp create cmd containerapp create added parameter service_principal_tenant_id

⚠️ 1006 - ParaAdd containerapp create cmd containerapp create added parameter source

⚠️ 1006 - ParaAdd containerapp create cmd containerapp create added parameter token

Greedygre · 2023-07-26T02:19:39Z

src/containerapp/azext_containerapp/containerapp_decorator.py


+        if self.get_argument_source():
+            app = self.set_up_create_containerapp_if_source_or_repo()
+            self.containerapp_def = self.set_up_create_containerapp_source(app=app)


As set_up_create_containerapp_source no need and has not return containerapp, here need to remove.

Suggested change

self.containerapp_def = self.set_up_create_containerapp_source(app=app)

self.set_up_create_containerapp_source(app=app)

snehapar9 · 2023-07-26T07:26:21Z

src/containerapp/azext_containerapp/_validators.py

+        if not registry_server or not registry_user or not registry_pass:
+            raise RequiredArgumentMissingError('Usage error: --registry-server, --registry-username and --registry-password are required while using --source or --repo.')
+    if repo and registry_server and "azurecr.io" in registry_server:
+        parsed = urlparse(registry_server)


@cormacpayne we should check this condition for source also?

@snehapar9 we should since we have the condition above to validate that registry_server is provided when source is provided -- we should be able to indent this entire block and remove the repo and registry_server checks to make it straightforward:

if source or repo: if not registry_server or not registry_user or not registry_pass: raise RequiredArgumentMissingError('Usage error: --registry-server, --registry-username and --registry-password are required while using --source or --repo.') if "azurecr.io" in registry_server: parsed = urlparse(registry_server) registry_name = (parsed.netloc if parsed.scheme else parsed.path).split(".")[0] if registry_name and len(registry_name) > MAXIMUM_SECRET_LENGTH: raise ValidationError(f"--registry-server ACR name must be less than {MAXIMUM_SECRET_LENGTH} " "characters when using --repo")```

snehapar9 · 2023-09-13T16:21:11Z

Closing this PR. Changes in this PR were merged into this PR

Add --source and --repo to containerapp create

887ecf6

azure-client-tools-bot-prd bot added this to the July 2023 (2023-08-01) milestone Jun 30, 2023

ghost requested a review from yonzhan June 30, 2023 15:54

ghost added the Auto-Assign Auto assign by bot label Jun 30, 2023

ghost requested review from wangzelin007 and yanzhudd June 30, 2023 15:54

ghost assigned zhoxing-ms Jun 30, 2023

ghost added the ContainerApp label Jun 30, 2023

ghost requested review from JennyLawrance, Juliehzl, chinadragon0515, ebencarek, howang-ms, jijohn14, jsntcy, pagariyaalok, ruslany, sanchitmehta, torosent, vinisoto, vturecek and zhoxing-ms June 30, 2023 15:54

cormacpayne reviewed Jun 30, 2023

View reviewed changes

Greedygre reviewed Jul 3, 2023

View reviewed changes

snehapar9 added 3 commits July 6, 2023 09:13

Refactored code for containerapp create

011439a

Merge branch 'main' into snehapar/Add-source-and-repo-to-create

e9fd59b

Merge branch 'snehapar/add-source-and-repo-to-containerapp-create' in…

89241ca

…to snehapar/Add-source-and-repo-to-create

snehapar9 added 6 commits July 6, 2023 09:23

Removed extra source param

916259c

Addressed PR comments

1439120

Removed new line

fb7f4fe

Fixed linting errors

d18d6c7

Remove new line

78181aa

Minor fixes for GitHub repo params

d646bd1

cormacpayne suggested changes Jul 7, 2023

View reviewed changes

Addressed PR comments

56e68d4

Juliehzl reviewed Jul 10, 2023

View reviewed changes

Greedygre reviewed Jul 14, 2023

View reviewed changes

Merge branch 'main' into snehapar/Add-source-and-repo-to-create

1b70cf5

snehapar9 added 2 commits July 25, 2023 12:32

Add source back

3a809ca

Remove return statement

07e1be5

Greedygre reviewed Jul 26, 2023

View reviewed changes

Addressed PR comments

25862fb

snehapar9 commented Jul 26, 2023

View reviewed changes

snehapar9 added 5 commits July 26, 2023 12:30

Refactor validation

0b2098b

Revert validation change

3fd9fde

Add live_only to tests

557f05e

Merge branch 'main' into snehapar/Add-source-and-repo-to-create

c2fb2d6

Merge branch 'main' into snehapar/Add-source-and-repo-to-create

ef8c6df

snehapar9 closed this Sep 13, 2023

		app = self.set_up_create_containerapp_if_source_or_repo(containerapp_def=containerapp_def)
		containerapp_def = self.set_up_create_containerapp_source(app=app, containerapp_def=containerapp_def)

	self.containerapp_def = self.set_up_create_containerapp_source(app=app)
	self.set_up_create_containerapp_source(app=app)

containerapp - Add --source and --repo to containerapp create #6463

containerapp - Add --source and --repo to containerapp create #6463

Uh oh!

Conversation

snehapar9 commented Jun 30, 2023

Related command

General Guidelines

About Extension Publish

Uh oh!

azure-client-tools-bot-prd bot commented Jun 30, 2023

Uh oh!

azure-client-tools-bot-prd bot commented Jun 30, 2023

Uh oh!

yonzhan commented Jun 30, 2023

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snehapar9 Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snehapar9 Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

snehapar9 Jul 10, 2023 •

edited

Loading

snehapar9 Jul 10, 2023 •

edited

Loading

snehapar9 commented Jul 10, 2023 •

edited

Loading

Greedygre Jul 14, 2023 •

edited

Loading

azure-client-tools-bot-prd bot commented Jul 25, 2023 •

edited

Loading

cormacpayne Jul 26, 2023 •

edited

Loading