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 command for getcache, listcaches and updatecache #179

Closed
wants to merge 1 commit into from

Conversation

hiteshshahjee
Copy link
Contributor

Added get, list and update command for cache operations.

  • getcache
  • listcaches
  • updatecache

Fix for issue 178.

Also updated test cases for PR-176 with GET, VERIFY step.

@hiteshshahjee
Copy link
Contributor Author

@whitlockjc can you review this PR

@hiteshshahjee
Copy link
Contributor Author

@whitlockjc Thanks for your continued support while reviewing the older PR. Can you pl review this PR.

@whitlockjc
Copy link
Member

@DinoChiesa and/or @kurtkanaskie, wanna help me review this? LGTM but I'd love a few more eyes on it.

@hiteshshahjee
Copy link
Contributor Author

@whitlockjc @DinoChiesa @kurtkanaskie Could you review this?

@hiteshshahjee
Copy link
Contributor Author

Any other reviewers who can help to review this?

@kurtkanaskie
Copy link
Contributor

Hi @hiteshshahjee, I finally got around to reviewing, sorry for the delay.
There's a serious issue with update, in that it overwrites previous values to default values, then updates the values provided by the options.
For example, if I try to update just the cache description, it sets "expiryDate" to "12-31-9999".
Also, if I try to update the "timeoutInSec" to "300" it sets both "expiryDate" and "timeoutInSec".
"expirySettings": { "expiryDate": { "value": "12-31-9999" }, "timeoutInSec": { "value": "300" } }

It should first get the existing cache representation and then update any options provided.
It should also keep "expirySettings" correct (exclusive values) as it does when I update a cache in the UI.

BTW, the "expirySettings" is also an issue with createCache command.

@hiteshshahjee
Copy link
Contributor Author

@kurtkanaskie thanks for your time to review the PR. Pl find my comments.

if I try to update just the cache description, it sets "expiryDate" to "12-31-9999".

As per edge documentation, if user need to update existing cache then the user has to provide details for all fields. Single field/element update is not allowed. With apigeetool, we have 2 options. - Users should always provide all fields. - (or) apigeetool can do a getcache call and use these value while doing the update call.
update cache

Also, if I try to update the "timeoutInSec" to "300" it sets both "expiryDate" and "timeoutInSec".
"expirySettings": { "expiryDate": { "value": "12-31-9999" }, "timeoutInSec": { "value": "300" } }

updatecache expirySettings is replicating the approach from createcache command.
With PR-176, we added support to pass expirySettings while making createcache call. After multiple discussion we agreed that we will always pass default value/user provided for expiryDate. timeoutInSec is passed when provided by user.
Even though getcache return both expiryDate & timeoutInSec, in edge portal it only show timeoutInSec in the UI.
The other option could be to pass either of these to edge for both create/update cache, this was the original solution which was implemented with PR-176. Pl let know if we need to revisit these design and pass exclusive values for both create and update cache

@hiteshshahjee
Copy link
Contributor Author

Any update on this?

@kurtkanaskie
Copy link
Contributor

I think its reasonable to expect the user to provide details for all fields, as per Edge API behavior:

You must specify the complete definition of the cache, including the properties that you want to change and the ones that retain their current value. Any properties omitted from the request body are reset to their default value.

That implies that the Edge API assigns the defaults and apigeetool does not specify the default values. So the object updateCachePayload on line #41 lib/commands/updatecache.js should start out empty and be populated with values provided.

I suggest testing the results using apigeetool and Edge API with various permutations of omitted fields. The results should be the same.

@whitlockjc
Copy link
Member

@kurtkanaskie Where do you stand on this?

@kurtkanaskie
Copy link
Contributor

kurtkanaskie commented Apr 22, 2020

I've reviewed this request again and re-emphasize that it should be corrected not to set any deprecated elements or defaults in the create and update requests.
I've tested and verified create, delete and update with various expiry types (date, time of day and seconds).
deleteCache
Should use "json:true" on line 48.

I think the create and updated commands should be implemented using the following.

createCache

/* jshint node: true  */
'use strict';

var util = require('util');
var defaults = require('../defaults');
var command_utils = require('./command-utils')

var descriptor = defaults.defaultDescriptor({
  environment: {
    name: 'Environment',
    shortOption: 'e',
    required: true
  },
  cache: {
    name: 'Cache Resource',
    shortOption: 'z',
    required: true
  },
  description: {
    name: 'Cache description',
    required: false
  },
  cacheExpiryByDate: {
    name: 'Cache expiration by date (mm-dd-yyyy)',
    required: false
  },
  cacheExpiryInSecs: {
    name: 'Cache expiration in seconds',
    required: false
  },
  cacheExpiryTimeOfDay: {
    name: 'Cache expiration at time of day (hh:mm:ss)',
    required: false
  }
});

module.exports.descriptor = descriptor;

module.exports.run = function(opts, cb) {
  if (opts.debug) {
    console.log('create: %j', opts);
  }

  var cachePayload  = { "name" : opts.cache };

  if(opts.cacheExpiryByDate && opts.cacheExpiryInSecs || 
     opts.cacheExpiryByDate && opts.cacheExpiryTimeOfDay ||
     opts.cacheExpireTimeOfDay && opts.cacheExpiryInSecs) {
    done(new Error('Expiry options are mutually exclusive'));
  }
  if( !opts.cacheExpiryByDate && !opts.cacheExpiryTimeOfDay && !opts.cacheExpiryInSecs) {
    done(new Error('Expiry option must be specified'));
  }

  if( opts.description) {
    cachePayload.description = opts.description;
  }

  if(opts.cacheExpiryByDate){
    cachePayload.expirySettings = {
      expiryDate: {
        value: opts.cacheExpiryByDate
      }
    }
  }
  if(opts.cacheExpiryInSecs){
    cachePayload.expirySettings = {
      timeoutInSec: {
        value: opts.cacheExpiryInSecs
      }
    }
  }
  if(opts.cacheExpiryTimeOfDay){
    cachePayload.expirySettings = {
      timeOfDay: {
        value: opts.cacheExpiryTimeOfDay
      }
    }
  }

  var uri = util.format('%s/v1/o/%s/e/%s/caches', opts.baseuri, opts.organization, opts.environment);
  var requestOptions = {
    uri: uri,
    method: 'POST',
    body: cachePayload,
    json: true
  }
  command_utils.run('createcache', opts, requestOptions, cb)
};

updateCache

/* jshint node: true  */
'use strict';

var util = require('util');
var defaults = require('../defaults');
var command_utils = require('./command-utils')

var descriptor = defaults.defaultDescriptor({
  environment: {
    name: 'Environment',
    shortOption: 'e',
    required: true
  },
  cache: {
    name: 'Cache Resource',
    shortOption: 'z',
    required: true
  },
  description: {
    name: 'Cache description',
    required: false
  },
  cacheExpiryByDate: {
    name: 'Cache expiration by date (mm-dd-yyyy)',
    required: false
  },
  cacheExpiryInSecs: {
    name: 'Cache expiration in seconds',
    required: false
  },
  cacheExpiryTimeOfDay: {
    name: 'Cache expiration at time of day (hh:mm:ss)',
    required: false
  }
});

module.exports.descriptor = descriptor;

module.exports.run = function(opts, cb) {
  if (opts.debug) {
    console.log('update: %j', opts);
  }

  var cachePayload  = { "name" : opts.cache };

  if(opts.cacheExpiryByDate && opts.cacheExpiryInSecs || 
     opts.cacheExpiryByDate && opts.cacheExpiryTimeOfDay ||
     opts.cacheExpireTimeOfDay && opts.cacheExpiryInSecs) {
    done(new Error('Expiry options are mutually exclusive'));
  }
  if( !opts.cacheExpiryByDate && !opts.cacheExpiryTimeOfDay && !opts.cacheExpiryInSecs) {
    done(new Error('Expiry option must be specified'));
  }

  if( opts.description) {
    cachePayload.description = opts.description;
  }

  if(opts.cacheExpiryByDate){
    cachePayload.expirySettings = {
      expiryDate: {
        value: opts.cacheExpiryByDate
      }
    }
  }
  if(opts.cacheExpiryInSecs){
    cachePayload.expirySettings = {
      timeoutInSec: {
        value: opts.cacheExpiryInSecs
      }
    }
  }
  if(opts.cacheExpiryTimeOfDay){
    cachePayload.expirySettings = {
      timeOfDay: {
        value: opts.cacheExpiryTimeOfDay
      }
    }
  }

  var uri = util.format('%s/v1/o/%s/e/%s/caches/%s', opts.baseuri, opts.organization, opts.environment, opts.cache);
  var requestOptions = {
    uri: uri,
    method: 'PUT',
    body: cachePayload,
    json: true
  }
  command_utils.run('updatecache', opts, requestOptions, cb)
};

@DinoChiesa
Copy link
Collaborator

I've added new commands for get, list, and clear cache, via #227 .
and tests.
I'm going to close this. I haven't added an updateCache, not sure we need that. Please open a new PR if you think that's required.
Thanks for your contribution.

@DinoChiesa DinoChiesa closed this May 6, 2022
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.

4 participants