-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
cleanup validations for VPN connection creation #9195
cleanup validations for VPN connection creation #9195
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.19 #9195 +/- ##
============================================
- Coverage 15.08% 15.08% -0.01%
- Complexity 11182 11183 +1
============================================
Files 5402 5402
Lines 473137 473148 +11
Branches 61094 61020 -74
============================================
Hits 71364 71364
- Misses 393835 393846 +11
Partials 7938 7938
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, this may need s2s testing if not already covered by a smoketest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLGTM.
Just one remark, maybe we can use a unique error message in the getAndValidate
methods.
server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java
Outdated
Show resolved
Hide resolved
The calls have different messages passed (though only slightly). Can you specify what you would like to see there @hsato03 ? |
What about and This should cover all cases and the |
5aaa6d4
to
215c5f4
Compare
@hsato03 applied your suggestion and did some extra cleanup. |
server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor suggestions, clgtm
@kiranchavala it was only caught/solved in the API. It is now also the UI. |
@blueorangutan package |
@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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11208 |
[SF] Trillian test result (tid-11568)
|
Co-authored-by: Henrique Sato <henriquesato2003@gmail.com>
Co-authored-by: Suresh Kumar Anaparti <sureshkumar.anaparti@gmail.com>
64aa6e2
to
c9a722b
Compare
@blueorangutan package |
@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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11242 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. It handles the error condition now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
Description
This PR adds a validation when creating a VPN connection in a VPC to make sure the VPN gateway exists.
Fixes: #7250
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?