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

fix: added a dummy package #1527

Merged
merged 1 commit into from
Nov 23, 2021

Conversation

elenaizaguirre
Copy link
Contributor

@elenaizaguirre elenaizaguirre commented Nov 9, 2021

added a dummy package for tests about install packages from github
sources required for #1210

Signed-off-by: Elena Izaguirre e.izaguirre.equiza@accenture.com

@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2021

Codecov Report

Merging #1527 (35412e8) into main (93360e1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1527   +/-   ##
=======================================
  Coverage   70.04%   70.04%           
=======================================
  Files         368      368           
  Lines       14108    14108           
  Branches     1701     1701           
=======================================
  Hits         9882     9882           
  Misses       3298     3298           
  Partials      928      928           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93360e1...35412e8. Read the comment docs.

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@elenaizaguirre Quick question: Do we need the .d.ts files as well? And how about the openapi.json and the generated code files?
You see where this is going: I want to reduce the 32 files in the diff to the absolute minimum viable dummy package (which might be as low as two files, the package.json and an index.js) but I might be overlooking one (or many) things so that's why this is a question not necessarily a change request.

@takeutak
Copy link
Contributor

@elenaizaguirre Thanks for contributing. Would you add the corresponding issue on Cactus GitHub? (If there is no corresponding issue, would you post the issue?)

@elenaizaguirre
Copy link
Contributor Author

@elenaizaguirre Quick question: Do we need the .d.ts files as well? And how about the openapi.json and the generated code files? You see where this is going: I want to reduce the 32 files in the diff to the absolute minimum viable dummy package (which might be as low as two files, the package.json and an index.js) but I might be overlooking one (or many) things so that's why this is a question not necessarily a change request.

@petermetz I have deleted *.d.ts files but I think openapi.json and generated code are necessary because the first one is required for some files and generated code is used because when we instantiate a plugin from the api server, it ensures that the plugin can call some methods:

    const pluginPackage = require(/* webpackIgnore: true */ packagePath); // this one is our dummy package
    const createPluginFactory = pluginPackage.createPluginFactory as PluginFactoryFactory;
    const pluginFactoryOptions: IPluginFactoryOptions = {
      pluginImportType: pluginImport.type,
    };
    const pluginFactory = await createPluginFactory(pluginFactoryOptions);
    const plugin = await pluginFactory.create(pluginOptions);
    try {
      await plugin.onPluginInit();

It also calls installOpenapiValidationMiddleware with each endpoint of the plugin.

@elenaizaguirre
Copy link
Contributor Author

@elenaizaguirre Thanks for contributing. Would you add the corresponding issue on Cactus GitHub? (If there is no corresponding issue, would you post the issue?)

@takeutak Is not an issue by itself, it is the first step for a Peter suggestion #1456 (comment). Anyway, I've added the original issue number to the description

@petermetz
Copy link
Contributor

petermetz commented Nov 16, 2021

@petermetz I have deleted *.d.ts files but I think openapi.json and generated code are necessary because the first one is required for some files and generated code is used because when we instantiate a plugin from the api server, it ensures that the plugin can call some methods:

@elenaizaguirre Could we just not have the plugin implement web services at all? If we limit our scope to the dummy package just being the testing instrument for verifying that the local file system path based plugin installation works then it doesn't matter if it's a web svc plugin or not IMO.

I mean something like this in an index.js file:
(pseudo-ish code, syntax might have small issues, some more methods are probably missing but the point is that we only need to provide the simplest possible implementation that does not make it crash at install & instantiation time to verify that it can be installed from the local file-system without a crash)

class DummyPlugin {
  onPluginInit() {
    console.log("DummyPlugin#onPluginInit() OK");
    return Promise.resolve();
  }
}

class DummyPluginFactory {
  create() {
    return Promise.resolve(new DummyPlugin());
  }
}

module.exports.createPluginFactory = (options) => {
  return Promise.resolve(new DummyPluginFactory(options));
};

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

^^ See above

@elenaizaguirre elenaizaguirre marked this pull request as draft November 17, 2021 16:31
@elenaizaguirre
Copy link
Contributor Author

@petermetz I have deleted *.d.ts files but I think openapi.json and generated code are necessary because the first one is required for some files and generated code is used because when we instantiate a plugin from the api server, it ensures that the plugin can call some methods:

@elenaizaguirre Could we just not have the plugin implement web services at all? If we limit our scope to the dummy package just being the testing instrument for verifying that the local file system path based plugin installation works then it doesn't matter if it's a web svc plugin or not IMO.

I mean something like this in an index.js file: (pseudo-ish code, syntax might have small issues, some more methods are probably missing but the point is that we only need to provide the simplest possible implementation that does not make it crash at install & instantiation time to verify that it can be installed from the local file-system without a crash)

class DummyPlugin {
  onPluginInit() {
    console.log("DummyPlugin#onPluginInit() OK");
    return Promise.resolve();
  }
}

class DummyPluginFactory {
  create() {
    return Promise.resolve(new DummyPlugin());
  }
}

module.exports.createPluginFactory = (options) => {
  return Promise.resolve(new DummyPluginFactory(options));
};

@petermetz Thanks, you were right, it was much simpler than I thought.

@elenaizaguirre elenaizaguirre marked this pull request as ready for review November 17, 2021 16:59
added a dummy package for tests about install packages from github
sources required for hyperledger-cacti#1210

Signed-off-by: Elena Izaguirre <e.izaguirre.equiza@accenture.com>
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@elenaizaguirre My pleasure and thank you very much for taking the time to address the nits as usual!

@petermetz petermetz merged commit e1e8aee into hyperledger-cacti:main Nov 23, 2021
@elenaizaguirre elenaizaguirre deleted the add-dummy-package branch November 23, 2021 08:34
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