Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Document how to pass CIDRs lists API calls#4035

Merged
DaanHoogland merged 1 commit intoapache:masterapache/cloudstack:masterfrom
onitake:doc/cirdlist-separatorCopy head branch name to clipboard
Jul 29, 2020
Merged

Document how to pass CIDRs lists API calls#4035
DaanHoogland merged 1 commit intoapache:masterapache/cloudstack:masterfrom
onitake:doc/cirdlist-separatorCopy head branch name to clipboard

Conversation

@onitake
Copy link
Contributor

@onitake onitake commented Apr 16, 2020

Description

This patch adds a sentence to all CIDR_LIST and DEST_CIDR_LIST API parameter and return value annotations that documents how to pass a list of CIDRs.

This is a rarely used feature, but may be necessary for certain scenarios, such as automatic firewall rule generation/matching, VPNs with multiple routed networks, etc.

There may be other cases where the APIs accept lists, but this was out of scope for this PR.

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)

Screenshots (if appropriate):

How Has This Been Tested?

I tested what values the API calls createFirewallRule and createLoadBalancerRule accept for the cidrlist parameter, and what is returned in listFirewallRules and listLoadBalancerRules.

Anything besides a single comma is not accepted.

Furthermore, createFirewallRule returns an error because the parameter is deprecated.

@onitake
Copy link
Contributor Author

onitake commented Apr 16, 2020

The integration test reports:

The job exceeded the maximum log length, and has been terminated.

Does that mean the result can be ignored, or is there something I need to look into?

@DaanHoogland
Copy link
Contributor

The integration test reports:

The job exceeded the maximum log length, and has been terminated.

Does that mean the result can be ignored, or is there something I need to look into?

You can go to travis and restart the particular run, it may pass next time, (due to more cpu?) as it logs less async queries for instance, or logs less host pings.

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.

I know this c&p pattern has been applied all over the code, but can you please create a constant with the test and refer that in those description instead of this, please?
the text itself makes perfect sense and would be a great help to users.

@onitake
Copy link
Contributor Author

onitake commented Apr 17, 2020

I can do that, once I figure out where to put this constant.
Hopefully it won't break the API doc parsing scripts.

@DaanHoogland
Copy link
Contributor

@onitake good point, i had not thought of that (yet)

@onitake
Copy link
Contributor Author

onitake commented Apr 17, 2020

Hm... It doesn't look like I can rerun builds on Travis.

@GabrielBrascher
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Copy link
Member

@GabrielBrascher GabrielBrascher left a comment

Choose a reason for hiding this comment

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

Code LGTM, simple description changes.

@blueorangutan
Copy link

Packaging result: ✔centos7 ✔debian. JID-1583

@DaanHoogland
Copy link
Contributor

textual changes only no furhter testing needed.

@DaanHoogland DaanHoogland merged commit c856614 into apache:master Jul 29, 2020
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.

6 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.