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

fix: change encoder to b64 #12

Merged
merged 1 commit into from
Dec 6, 2024
Merged

fix: change encoder to b64 #12

merged 1 commit into from
Dec 6, 2024

Conversation

NichArchA82
Copy link
Collaborator

@NichArchA82 NichArchA82 commented Dec 6, 2024

PR Type

bug_fix


Description

  • Fixed encoding and decoding of VM metadata and descriptions by switching from encoder to b64.
  • Ensured consistent use of base64 encoding for virtual machine operations.

Changes walkthrough 📝

Relevant files
Bug fix
main.py
Update encoder to base64 for metadata description               

app/main.py

  • Changed encoder from encoder to b64 for encoding metadata description.

  • +1/-1     
    virsh.py
    Update decoder to base64 for VM descriptions                         

    app/util/virsh.py

  • Changed encoder from encoder to b64 for decoding VM descriptions.
  • +1/-1     

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

    Copy link
    Contributor

    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

    Breaking Change
    Switching from encoder to b64 for encoding metadata could break compatibility with existing VM metadata. Need to verify if existing VMs' metadata can still be decoded properly.

    Error Handling
    The decoding of VM description lacks error handling. If the description is malformed or not base64 encoded, this could raise unhandled exceptions.

    @NichArchA82 NichArchA82 merged commit d2a376e into main Dec 6, 2024
    3 checks passed
    @NichArchA82 NichArchA82 deleted the fix/list-servers branch December 6, 2024 09:17
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for potential base64 encoding failures to prevent runtime crashes

    Add error handling for the base64 encoding operation since it could fail if the JSON
    string contains invalid characters.

    app/main.py [96]

    -metadata_description=b64.encode_string(json.dumps(vm.tags)),
    +try:
    +    metadata_description=b64.encode_string(json.dumps(vm.tags)),
    +except Exception as e:
    +    logger.error(f"Failed to encode VM tags: {e}")
    +    raise HTTPException(status_code=400, message="Invalid VM tags format")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for base64 encoding is crucial as it prevents potential runtime crashes and provides clear error messages when VM tags contain invalid characters. The suggestion includes proper logging and user feedback via HTTP exceptions.

    8
    Handle potential base64 decoding failures for malformed VM descriptions

    Add error handling for the base64 decoding operation since it could fail if the
    stored description is malformed.

    app/util/virsh.py [101]

    -'description': json.loads(b64.decode_string(describe_vm(connect, name)))
    +'description': json.loads(b64.decode_string(describe_vm(connect, name))) if describe_vm(connect, name) else {}
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While error handling for decoding is important, the suggested solution using a conditional return of empty dict is overly simplistic and doesn't properly handle or log decoding errors. A more comprehensive error handling approach would be more beneficial.

    4

    💡 Need additional feedback ? start a PR chat

    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.

    2 participants