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

Support init container images #132

Merged
merged 3 commits into from
Mar 20, 2025
Merged

Support init container images #132

merged 3 commits into from
Mar 20, 2025

Conversation

adam-duplocloud
Copy link
Contributor

@adam-duplocloud adam-duplocloud commented Mar 20, 2025

User description

Describe Changes

See commit messages and code comments for more details. It's worth reading the comments for the "two of one" cases closely.

To run the tests I added:

pytest src/tests/test_service_update_image.py

This increases coverage from 14% to 27% on the service module.

Aside from the unit tests, I created a temp service in a Duplo portal and updated its service image, additional container images, and init container images in several patterns.

Link to Issues

N/A.

PR Review Checklist

  • Thoroughly reviewed on local machine.
  • Have you added any tests
  • Make sure to note changes in Changelog

PR Type

Enhancement, Tests, Documentation


Description

  • Added support for updating init container images in services.

  • Enhanced update_image method to handle init containers.

  • Introduced comprehensive unit tests for init container functionality.

  • Updated documentation and changelog to reflect new features.


Changes walkthrough 📝

Relevant files
Enhancement
service.py
Support init container updates in `update_image`                 

src/duplo_resource/service.py

  • Added init_container_image argument to update_image method.
  • Enhanced logic to handle init container updates.
  • Improved error messages for container updates.
  • +19/-13 
    args.py
    Add CLI argument for init container images                             

    src/duplocloud/args.py

    • Added INIT_CONTAINER_IMAGE argument for init container updates.
    +6/-0     
    Tests
    test_service_update_image.py
    Add unit tests for init container updates                               

    src/tests/test_service_update_image.py

  • Added unit tests for init container image updates.
  • Validated error handling for invalid arguments.
  • Tested various scenarios for init and additional containers.
  • +429/-0 
    Documentation
    CHANGELOG.md
    Update changelog for init container support                           

    CHANGELOG.md

    • Documented support for init containers in update_image.
    +4/-0     
    CONTRIBUTING.md
    Enhance contributing guide with setup instructions             

    CONTRIBUTING.md

  • Added instructions for setting up a virtual environment.
  • Improved guidance for running unit and integration tests.
  • +19/-1   
    Dependencies
    pyproject.toml
    Update test dependencies for mocking                                         

    pyproject.toml

    • Added pytest-mock to test dependencies.
    +2/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The PR allows updating multiple container types, but the error handling for the "two of one" case (trying to update containers that don't exist) could be improved. Currently, it only raises an error if no containers are updated, but silently ignores non-existent containers if at least one is updated.

    if not updated_containers:
      raise DuploError(f"No matching containers found in service '{name}'")
    Edge Case Testing

    The tests acknowledge a potential issue with the "two of one" cases (lines 232-235 and 386-390), where trying to update multiple containers when only one exists doesn't raise an error. Consider whether this behavior should be changed.

    # Update two of one additional containers.
    # Arguably, trying to update images for two additional containers in a service that only has one additional container
    # should raise an error. This test was written to assert existing behavior to preserve backwards compatibility.
    (
      {
        'Template': {'OtherDockerConfig': json.dumps({'additionalContainers': [
          {
            'name': 'widget-sidecar2',
            'image': 'widget:v1',
          }
        ]})}
      },
      {
        'name': 'widget',
        'container_image': [('widget-sidecar1', 'widget:v2'), ('widget-sidecar2', 'widget:v2')]
      },
      {
        'Name': 'widget',
        'OtherDockerConfig': json.dumps({'additionalContainers': [
          {
            'name': 'widget-sidecar2',
            'image': 'widget:v2',
          }
        ]}),
        'AllocationTags': '',
      }
    ),

    @duploctl
    Copy link
    Contributor

    duploctl bot commented Mar 20, 2025

    ☂️ Python Coverage

    current status: ✅

    Overall Coverage

    Lines Covered Coverage Threshold Status
    2435 520 21% 0% 🟢

    New Files

    No new covered files...

    Modified Files

    File Coverage Status
    src/duplo_resource/service.py 27% 🟢
    src/duplocloud/args.py 100% 🟢
    TOTAL 63% 🟢

    updated for commit: 6e072e3 by action🐍

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 20, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix parameter validation logic
    Suggestion Impact:The commit implemented a similar validation logic to what was suggested, but with a different approach. The suggestion wanted to check that at least one parameter is provided and no more than one parameter is provided. The commit implemented this by removing the init_container_image parameter entirely and explicitly checking that either image OR container_image is provided (but not both).

    code diff:

    -    if [image, container_image, init_container_image].count(None) != 2:
    -      raise DuploError("Provide a service image, container images, or init container images.")
    +    if image and container_image:
    +      raise DuploError("Provide either <service-image> OR --container-image <side-car-container> <container-image>, but not both.")
    +    if not image and not container_image:
    +      raise DuploError("Provide either <service-image> or --container-image <side-car-container> <container-image>.")

    The current condition is incorrect. It requires exactly one of the three
    parameters to be non-None, but doesn't properly handle the case when all three
    are None. Change the condition to explicitly check that at least one parameter
    is provided.

    src/duplo_resource/service.py [249-250]

    -if [image, container_image, init_container_image].count(None) != 2:
    +if not any([image, container_image, init_container_image]):
       raise DuploError("Provide a service image, container images, or init container images.")
    +if sum(x is not None for x in [image, container_image, init_container_image]) > 1:
    +  raise DuploError("Provide only one of: service image, container images, or init container images.")

    [Suggestion has been applied]

    Suggestion importance[1-10]: 9

    __

    Why: The current validation logic is flawed. It checks if exactly one parameter is non-None, but fails to properly handle the case when all parameters are None. The improved code correctly validates that exactly one parameter is provided, preventing potential runtime errors.

    High
    • Update

    @adam-duplocloud
    Copy link
    Contributor Author

    The bot is misinterpreting the "two of one" case. That was pre-existing and I added tests to assert it for backwards compatibility. Not introduced in this PR.

    Copy link
    Contributor

    @duplocloud-matt duplocloud-matt left a comment

    Choose a reason for hiding this comment

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

    LGTM!

    This includes adding test coverage for existing behavior to
    preserve backwards-compatibility.
    
    This was written to be a minimal addition to the existing
    support for 'additional containers', which work essentially the
    same as 'init containers'. To keep the changes minimal, unit
    tests rely on mocks and patches instead of rewriting the code
    to be more easily unit testable.
    @adam-duplocloud adam-duplocloud force-pushed the support-init-container-images branch from ec62d2b to 6e072e3 Compare March 20, 2025 18:57
    Copy link
    Collaborator

    @kferrone kferrone left a comment

    Choose a reason for hiding this comment

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

    lgtm

    @kferrone kferrone merged commit e822714 into main Mar 20, 2025
    5 checks passed
    @kferrone kferrone deleted the support-init-container-images branch March 20, 2025 20:12
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants