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

Added additional params for createcache. Allows description and expiry settings to be passed as arguments #176

Merged
merged 7 commits into from
Nov 11, 2019

Conversation

hiteshshahjee
Copy link
Contributor

Added additional params for createcache commands. Allows description and expiry settings to be passed as arguments.

Currently description and expiry settings arguments are hard-coded for createcache command in the code. PR changes is to allow description and expiry settings to be passed as arguments for createcache command.

Fix for issues
Issue 174
Changes are tested with sample commands

apigeetool createcache -u [email protected] -o sdoe -e test -z test-cache --description "sample key" --cacheExpiryByDate 12-31-9999

apigeetool createcache -u [email protected] -o sdoe -e test -z test-cache --description "sample key" --cacheExpiryInSecs 40000

@hiteshshahjee
Copy link
Contributor Author

@whitlockjc can you pl review the PR

Copy link
Member

@whitlockjc whitlockjc left a comment

Choose a reason for hiding this comment

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

Overall, I like where you're going but I don't think it makes sense to treat previously optional (as in they are not required by the Edge MGMT API 1 and they weren't providable by the user prior for apigeetool) options as required. If you treat your new options as optional, the logic behind applying the values (when provided) should then change to match. Ideally, your new options would override previously hard-coded values if/when provided by the user but the CLI interface in its requirements shouldn't change.

Make sense?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/commands/createcache.js Outdated Show resolved Hide resolved
lib/commands/createcache.js Outdated Show resolved Hide resolved
lib/commands/createcache.js Outdated Show resolved Hide resolved
@hiteshshahjee
Copy link
Contributor Author

@whitlockjc thanks for your comments. Updated PR to use default values for description and expiry.

Copy link
Member

@whitlockjc whitlockjc left a comment

Choose a reason for hiding this comment

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

Thanks for your continued work, but there are a couple more changes to make please.

lib/commands/createcache.js Outdated Show resolved Hide resolved
lib/commands/createcache.js Outdated Show resolved Hide resolved
lib/commands/createcache.js Outdated Show resolved Hide resolved
lib/commands/createcache.js Outdated Show resolved Hide resolved
@hiteshshahjee
Copy link
Contributor Author

@whitlockjc thanks for your continued support for reviewing the PR.
PR is updated to pass both cacheExpiryByDate and cacheExpiryInSecs to Edge MGMT API.

Summary for different scenarios

  • No expiry settings are provided by customer (CLI consumer), then default value for cacheExpiryByDate (default date 12-31-9999) will be sent to Edge MGMT API. Edge MGMT API stores cacheExpiryByDate.

  • Customer (CLI consumer) provides cacheExpiryByDate, then only cacheExpiryByDate (user input) will be send to Edge MGMT API. Edge MGMT API stores cacheExpiryByDate.

  • Customer (CLI consumer) provides cacheExpiryInSecs, then both cacheExpiryByDate (default date 12-31-9999) and cacheExpiryInSecs (user input) will be send to Edge MGMT API. Edge MGMT API stores cacheExpiryInSecs only.

  • Customer (CLI consumer) provides cacheExpiryByDate and cacheExpiryInSecs, then both cacheExpiryByDate (user input) and cacheExpiryInSecs (user input) will be send to Edge MGMT API. Edge MGMT API stores cacheExpiryInSecs only.

Pl review and provide your feedback.

Copy link
Member

@whitlockjc whitlockjc left a comment

Choose a reason for hiding this comment

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

One more nit, also...any tests?

lib/commands/createcache.js Outdated Show resolved Hide resolved
@hiteshshahjee
Copy link
Contributor Author

@whitlockjc Added test case for createcache with expiry settings. Pl review.

Copy link
Member

@whitlockjc whitlockjc left a comment

Choose a reason for hiding this comment

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

Thank you for your patience here, I really appreciate your contribution. A few more minor changes and we should be ready to go.

lib/commands/createcache.js Outdated Show resolved Hide resolved
remotetests/remotetest.js Outdated Show resolved Hide resolved
lib/commands/createcache.js Outdated Show resolved Hide resolved
lib/commands/createcache.js Outdated Show resolved Hide resolved
remotetests/remotetest.js Outdated Show resolved Hide resolved
@hiteshshahjee
Copy link
Contributor Author

PR updated with test cases for below scenarios.

  • Create cache with cacheExpiryByDate
  • Create cache with cacheExpiryInSecs
  • Create cache with both cacheExpiryByDate and cacheExpiryInSecs

@whitlockjc
Copy link
Member

Since I don't see the changes to check the cache information for the provided arguments, is your plan to update these tests when you submit your PR for getting cache information?

@hiteshshahjee
Copy link
Contributor Author

Yes, will update these test to add get , verify as part of getting cache information PR

@hiteshshahjee
Copy link
Contributor Author

@whitlockjc Did you get chance to review these test cases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants