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

DUPLO-21449: Added --wait flag for Service Start, Stop and Restart #120

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

shantanuduplo
Copy link
Contributor

@shantanuduplo shantanuduplo commented Jan 8, 2025

User description

Describe Changes

Added --wait flag for Service Start, Stop and Restart

Link to Issues

https://app.clickup.com/t/8655600/DUPLO-21449

PR Review Checklist

  • Thoroughly reviewed on local machine.
  • Make sure to note changes in Changelog.

PR Type

Enhancement


Description

  • Added --wait flag for service start, stop, and restart commands.

  • Updated restart, stop, and start methods to handle wait flag.

  • Enhanced CLI commands to optionally wait for service updates.

  • Updated changelog to reflect new --wait flag feature.


Changes walkthrough 📝

Relevant files
Enhancement
service.py
Add `wait` flag support to service methods                             

src/duplo_resource/service.py

  • Added wait parameter to restart, stop, and start methods.
  • Updated method logic to handle waiting for service updates.
  • Enhanced docstrings to include wait parameter details.
  • +19/-4   
    Documentation
    CHANGELOG.md
    Update changelog for `--wait` flag feature                             

    CHANGELOG.md

    • Documented the addition of --wait flag for service commands.
    +1/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @shantanuduplo shantanuduplo requested a review from kferrone January 8, 2025 07:28
    @zafarabbas
    Copy link

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 8, 2025

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling

    The wait functionality should handle timeout scenarios and service update failures. Consider adding timeout parameter and error handling for wait operations.

    if wait:
      service = self.find(name)
      self.wait(service, service)
    Typo

    The endpoint URL for start operation contains incorrect casing - 'ReplicationControllerstart' should be 'ReplicationControllerStart' to match the pattern used in other endpoints.

    self.duplo.post(self.endpoint(f"ReplicationControllerstart/{name}"))

    @duploctl
    Copy link
    Contributor

    duploctl bot commented Jan 8, 2025

    ☂️ Python Coverage

    current status: ✅

    Overall Coverage

    Lines Covered Coverage Threshold Status
    2293 445 19% 0% 🟢

    New Files

    No new covered files...

    Modified Files

    File Coverage Status
    src/duplo_resource/service.py 15% 🟢
    TOTAL 15% 🟢

    updated for commit: 7867368 by action🐍

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 8, 2025

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix inconsistent API endpoint casing that could cause request failures

    The endpoint URL contains a capitalization error - 'ReplicationControllerstart'
    should be 'ReplicationControllerStart' to match the casing pattern used in other
    endpoints.

    src/duplo_resource/service.py [487]

    -self.duplo.post(self.endpoint(f"ReplicationControllerstart/{name}"))
    +self.duplo.post(self.endpoint(f"ReplicationControllerStart/{name}"))
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The inconsistent casing in the API endpoint URL is a critical issue that could cause runtime failures. The pattern 'ReplicationController[Action]' is used consistently elsewhere, making this a high-priority fix.

    9
    General
    Add existence validation before performing operations on resources

    Add error handling to verify the service exists before attempting operations that
    could fail.

    src/duplo_resource/service.py [465-467]

     def start(self, 
               name: args.NAME,
               wait: args.WAIT = False) -> dict:
    +    if not self.find(name):
    +        raise DuploError(f"Service '{name}' not found")
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding upfront service existence validation would prevent unclear API errors and provide better user feedback, improving the robustness of the service operations.

    7
    Improve state comparison logic in wait functionality to properly track service changes

    The wait functionality passes the same service object twice to the wait method,
    which seems incorrect. Consider passing appropriate before/after states.

    src/duplo_resource/service.py [430-432]

     if wait:
    -  service = self.find(name)
    -  self.wait(service, service)
    +  before = self.find(name)
    +  after = self.find(name)
    +  self.wait(before, after)
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While passing the same service object twice to wait() seems redundant, the suggestion's improvement doesn't fundamentally fix the issue as it still fetches the service state twice at the same time.

    4

    @shantanuduplo shantanuduplo merged commit b86744c into main Jan 9, 2025
    5 checks passed
    @shantanuduplo shantanuduplo deleted the DUPLO-21449 branch January 9, 2025 05:51
    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