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] Bug fix update container name #7623

Merged
merged 40 commits into from
May 16, 2024

Conversation

snehapar9
Copy link
Contributor


This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

This PR ensures the container name is updated after running update in source to cloud flow.

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? (pip install wheel==0.30.0 required)
  • My extension version conforms to the Extension version schema

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.

snehapar9 and others added 30 commits November 3, 2023 14:42
Copy link

github-actions bot commented May 15, 2024

⚠️ Release Suggestions

Module: containerapp

  • Update VERSION to 1.0.0b1 in src/containerapp/setup.py

Notes

  • Stable/preview tag is inherited from last release. If needed, please add stable/preview label to modify it.
  • Major/minor/patch/pre increment of version number is calculated by pull request code changes automatically. If needed, please add major/minor/patch/pre label to adjust it.
  • For more info about extension versioning, please refer to Extension version schema

@Greedygre
Copy link
Contributor

Hi @snehapar9
Please remove the file not related to the fix: containerapp-0.3.43-py2.py3-none-any.whl
Please add test to cover this fix.

@snehapar9
Copy link
Contributor Author

Hi @snehapar9 Please remove the file not related to the fix: containerapp-0.3.43-py2.py3-none-any.whl Please add test to cover this fix.

Thank you for catching that @Greedygre! Removed it

Copy link

@MoazzemHossain-bot MoazzemHossain-bot left a comment

Choose a reason for hiding this comment

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

Okay

@Greedygre
Copy link
Contributor

Please modify history.rst to add the description about this fix.

@yanzhudd
Copy link
Contributor

please resolve the code conflict

@yanzhudd
Copy link
Contributor

please add test for this change to verify if it can meet your expectations

@snehapar9
Copy link
Contributor Author

please add test for this change to verify if it can meet your expectations

Thanks, added test for this scenario and resolved conflicts.

Comment on lines 495 to 498
app = test_cls.cmd(f'containerapp show -g {resource_group} -n {name}').get_output_in_json()

# Verify that the Container App has the correct environment variables
test_cls.assertEqual(app["properties"]["template"]["containers"][0]["env"], [{"name": "testkey1", "value": "value1"}, {"name": "testkey2", "value": "value2"}])
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use checks=[] to check the properties

Copy link
Contributor

@Greedygre Greedygre May 16, 2024

Choose a reason for hiding this comment

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

        self.cmd('containerapp show -g {} -n {}'.format(resource_group, ca_name), checks=[
            JMESPathCheck('length(properties.template.containers[0].env[?name==`testkey1`])', "value1")
        ])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Comment on lines 502 to 505
app = test_cls.cmd(f'containerapp show -g {resource_group} -n {name}').get_output_in_json()

# Verify that the Container App has the correct environment variables
test_cls.assertEqual(app["properties"]["template"]["containers"][0]["env"], [{"name": "testkey1", "value": "value1"}, {"name": "testkey2", "value": "value2"}])
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@yanzhudd yanzhudd merged commit a4877f5 into Azure:main May 16, 2024
14 of 15 checks passed
Comment on lines +493 to +503
# Create and verify Container App using cloud build
test_cls.cmd(f'containerapp create -g {resource_group} -n {name} --environment {env_name} --source \"{source_path}\" --env-vars "testkey1=value1" "testkey2=value2"')
test_cls.cmd(f'containerapp show -g {resource_group} -n {name}', checks=[
JMESPathCheck('properties.template.containers[0].env', [{'name': 'testkey1', 'value': 'value1'}, {'name': 'testkey2', 'value': 'value2'}])
])

# Update and verify Container App using cloud build
test_cls.cmd(f'containerapp update -g {resource_group} -n {name} --source \"{source_path}\"')
test_cls.cmd(f'containerapp show -g {resource_group} -n {name}', checks=[
JMESPathCheck('properties.template.containers[0].env', [{'name': 'testkey1', 'value': 'value1'}, {'name': 'testkey2', 'value': 'value2'}])
])
Copy link
Contributor

@Greedygre Greedygre May 16, 2024

Choose a reason for hiding this comment

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

Seems you didn't check the container name? I think the container name is the what you want to check, right?

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.

7 participants