Skip to content

Conversation

@cheng102e
Copy link
Contributor

@cheng102e cheng102e commented Mar 29, 2024

Description

This PR...

clean magic value, and update if-else to switch

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

@boring-cyborg
Copy link

boring-cyborg bot commented Mar 29, 2024

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@codecov
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

❌ Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.60%. Comparing base (128dbe2) to head (d29fda7).
⚠️ Report is 20 commits behind head on 4.22.

Files with missing lines Patch % Lines
...-proxy/rdpconsole/src/main/java/common/Client.java 0.00% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               4.22    #8848   +/-   ##
=========================================
  Coverage     17.60%   17.60%           
- Complexity    15615    15616    +1     
=========================================
  Files          5911     5911           
  Lines        529966   529967    +1     
  Branches      64747    64748    +1     
=========================================
+ Hits          93285    93289    +4     
+ Misses       426180   426178    -2     
+ Partials      10501    10500    -1     
Flag Coverage Δ
uitests 3.60% <ø> (ø)
unittests 18.67% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9099

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

clgtm

@DaanHoogland
Copy link
Contributor

@blueorangutan test alma9 kvm-alma9

@blueorangutan
Copy link

@DaanHoogland a [SL] Trillian-Jenkins test job (alma9 mgmt + kvm-alma9) has been kicked to run smoke tests

@weizhouapache
Copy link
Member

suggestion:

use EnumUtils.getEnumIgnoreCase with a default value

@blueorangutan
Copy link

[SF] Trillian test result (tid-9656)
Environment: kvm-alma9 (x2), Advanced Networking with Mgmt server a9
Total time taken: 53600 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8848-t9656-kvm-alma9.zip
Smoke tests completed. 127 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_02_enableHumanReadableLogs Error 0.16 test_human_readable_logs.py
test_03_secured_to_nonsecured_vm_migration Error 155.35 test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration Error 274.39 test_vm_life_cycle.py

@DaanHoogland
Copy link
Contributor

suggestion:

use EnumUtils.getEnumIgnoreCase with a default value

@cheng102e , will you replace the switch statement?


private static final String VNC = "vnc";
private static final String RDP = "rdp";
private static final String HYPERV = "hyperv";
Copy link
Contributor

Choose a reason for hiding this comment

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

i think, no need of this string definitions, try with string to enum constant conversion?

@sureshanaparti sureshanaparti requested a review from Copilot June 5, 2025 10:03
@sureshanaparti
Copy link
Contributor

@blueorangutan package

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the protocol selection logic for the RDP console client by replacing magic-string comparisons with static constant values and updating an if-else chain to a switch statement.

  • Removed magic values by introducing constants for protocol names.
  • Replaced an if-else chain with a switch statement to improve clarity and maintainability.

@DaanHoogland
Copy link
Contributor

@cheng102e , will you still be looking at this (and answer #8848 (comment)) ?

Copy link
Contributor

@sureshanaparti sureshanaparti left a comment

Choose a reason for hiding this comment

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

minor nit, current code lgtm

@sureshanaparti
Copy link
Contributor

@blueorangutan package

@RosiKyu
Copy link
Collaborator

RosiKyu commented Oct 14, 2025

@blueorangutan package

@blueorangutan
Copy link

@rosi-shapeblue a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15448

@RosiKyu
Copy link
Collaborator

RosiKyu commented Oct 14, 2025

@blueorangutan test

@blueorangutan
Copy link

@rosi-shapeblue a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-14661)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 57653 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8848-t14661-kvm-ol8.zip
Smoke tests completed. 149 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

@rajujith
Copy link

@cheng102e Since this is for the 4.22.1 release, could you retarget the PR to the 4.22 branch?

@DaanHoogland DaanHoogland changed the base branch from main to 4.22 January 13, 2026 12:40
@sureshanaparti
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16552

@sureshanaparti
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 16561

@sureshanaparti
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@sureshanaparti a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-15306)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 49413 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8848-t15306-kvm-ol8.zip
Smoke tests completed. 149 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

@sureshanaparti sureshanaparti merged commit 062b98a into apache:4.22 Jan 28, 2026
25 of 26 checks passed
@boring-cyborg
Copy link

boring-cyborg bot commented Jan 28, 2026

Awesome work, congrats on your first merged pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

8 participants