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 for parameter cidrlist added to the UI #6869

Conversation

JoaoJandre
Copy link
Contributor

@JoaoJandre JoaoJandre commented Nov 3, 2022

Description

PR #6460 reimplemented the cidrlist in API createLoadBalancerRule; However, this parameter was not implemented in the UI. This PRs intends to implement the cidrlist field in the create load balancer rule form.

This PR also fixes the BulkActionView.vue, which currently throws an exception when used.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Screenshots (if appropriate):

image

How Has This Been Tested?

This was tested in a local lab, by running npm run serve and creating/deleting load balancer rules with cidrlists. The BulkActionView was also tested: before this change it was throwing an exception and was not opening the form. After the changes, it worked fine.

@acs-robot
Copy link
Collaborator

Found UI changes, kicking a new UI QA build
@blueorangutan ui

@blueorangutan
Copy link

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Merging #6869 (504e267) into main (fa39e61) will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #6869   +/-   ##
=========================================
  Coverage     10.84%   10.85%           
- Complexity     7104     7106    +2     
=========================================
  Files          2485     2485           
  Lines        245417   245417           
  Branches      38326    38326           
=========================================
+ Hits          26627    26631    +4     
+ Misses       215521   215515    -6     
- Partials       3269     3271    +2     
Impacted Files Coverage Δ
...rg/apache/cloudstack/quota/QuotaStatementImpl.java 40.26% <0.00%> (ø)
...dstack/network/contrail/model/ModelObjectBase.java 28.84% <0.00%> (+7.69%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@BryanMLima BryanMLima left a comment

Choose a reason for hiding this comment

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

LGTM

@DaanHoogland DaanHoogland reopened this Nov 8, 2022
@DaanHoogland
Copy link
Contributor

@blueorangutan ui

@blueorangutan
Copy link

@DaanHoogland a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@JoaoJandre
Copy link
Contributor Author

@blueorangutan ui

@blueorangutan
Copy link

@JoaoJandre a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@blueorangutan
Copy link

UI build: ✖️
(SL-JID-2652)

@DaanHoogland
Copy link
Contributor

@blueorangutan ui

@blueorangutan
Copy link

@DaanHoogland a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@blueorangutan
Copy link

UI build: ✖️
(SL-JID-2655)

@JoaoJandre
Copy link
Contributor Author

@blueorangutan ui

@blueorangutan
Copy link

@JoaoJandre a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@blueorangutan
Copy link

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/6869 (QA-JID-19)

Copy link
Contributor

@GutoVeronezi GutoVeronezi left a comment

Choose a reason for hiding this comment

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

CLGTM

@GutoVeronezi
Copy link
Contributor

Merging this one based on the approvals and build result.

@GutoVeronezi GutoVeronezi merged commit 440d780 into apache:main Dec 20, 2022
@weizhouapache
Copy link
Member

@GutoVeronezi @JoaoJandre @stephankruggg
Do you have QA engineer or mutually review/testing process ?

@DaanHoogland DaanHoogland added this to the 4.18.0.0 milestone Jan 5, 2023
@GutoVeronezi
Copy link
Contributor

@GutoVeronezi @JoaoJandre @stephankruggg Do you have QA engineer or mutually review/testing process ?

@weizhouapache, yes, we have a process; however, as you may know by personal experience, sometimes some use cases are missed. Thanks for fixing the regression, we will pay more attention in next PRs.

@weizhouapache
Copy link
Member

@GutoVeronezi
thanks, good to know. no worries, bugs could happen.

I have a suggestion: If any of you have tested a PR, can you comment the PR that you have tested it, for example "manually tested ok" ? It would be much better if test steps and results are added as well.

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.

None yet

8 participants