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

Injection plugins #2241

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Piankero
Contributor

Piankero commented Sep 17, 2018

I want to propose a plugin which injects errors in configurations. This is my first idea and some sort of architecture document.

If it is accepted I will also add all information to Metadata.ini and error/specification (so many new metadatas are very timeconsuming).

@markus2330

Thank you, looks nice.

Show outdated Hide outdated src/plugins/injection/README.md
Show outdated Hide outdated src/plugins/injection/README.md
## Usage
There up to 6 error types currently with injection types for each.
1. Structural errors (e.g. missing sections, parameters in wrong sections, omission necessary parameters in section)

This comment has been minimized.

@markus2330

markus2330 Sep 21, 2018

Contributor

Afaik some markdown renderer need a newline in between so that this enumeration is recognized.

@markus2330

markus2330 Sep 21, 2018

Contributor

Afaik some markdown renderer need a newline in between so that this enumeration is recognized.

Show outdated Hide outdated src/plugins/injection/README.md
[logger]
inject/semantic/#0 = pyLog
inject/semantic/#1 = log4net
inject/semantic/#2 = Log4js

This comment has been minimized.

@markus2330

markus2330 Sep 21, 2018

Contributor

Isn't this a capitalization error?

@markus2330

markus2330 Sep 21, 2018

Contributor

Isn't this a capitalization error?

This comment has been minimized.

@Piankero

Piankero Sep 22, 2018

Contributor

No, the program expects a logging framework for the java domain but actually the user gives a loggingframework for python/.NET/JS as he might not know the correct one.

@Piankero

Piankero Sep 22, 2018

Contributor

No, the program expects a logging framework for the java domain but actually the user gives a loggingframework for python/.NET/JS as he might not know the correct one.

@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Sep 23, 2018

Contributor

I changed everything. I also added another section concerning logging as I need to communicate which changes were done to lyrebird. Please take a short look over it if you also think this is a good idea.

Contributor

Piankero commented Sep 23, 2018

I changed everything. I also added another section concerning logging as I need to communicate which changes were done to lyrebird. Please take a short look over it if you also think this is a good idea.

## Logging of changes
To see which changes were applied to the whole KeySet, additional metadata will be written to the rootkey.

This comment has been minimized.

@markus2330

markus2330 Sep 30, 2018

Contributor

Description is very short, it is not yet clear how this logging should work. In METADATA.ini logging is already proposed, maybe you can extend this work so that logging is not specific to the inject plugin?

@markus2330

markus2330 Sep 30, 2018

Contributor

Description is very short, it is not yet clear how this logging should work. In METADATA.ini logging is already proposed, maybe you can extend this work so that logging is not specific to the inject plugin?

This comment has been minimized.

@Piankero

Piankero Sep 30, 2018

Contributor

In METADATA.ini logging is already proposed.

I do not see anything concerning logging in the metadata.ini

@Piankero

Piankero Sep 30, 2018

Contributor

In METADATA.ini logging is already proposed.

I do not see anything concerning logging in the metadata.ini

This comment has been minimized.

@Piankero

Piankero Sep 30, 2018

Contributor

Maybe log is the wrong wording as I just want to backtrace injections which were done (i.e. typo error on setting xy, structure error on key zx, etc.)

@Piankero

Piankero Sep 30, 2018

Contributor

Maybe log is the wrong wording as I just want to backtrace injections which were done (i.e. typo error on setting xy, structure error on key zx, etc.)

This comment has been minimized.

@markus2330

markus2330 Sep 30, 2018

Contributor

I do not see anything concerning logging in the metadata.ini

Sorry, it was removed in 06e10ca and did not really contain useful information.

Maybe log is the wrong wording as I just want to backtrace injections which were done

Yes, better to change the wording then.

@markus2330

markus2330 Sep 30, 2018

Contributor

I do not see anything concerning logging in the metadata.ini

Sorry, it was removed in 06e10ca and did not really contain useful information.

Maybe log is the wrong wording as I just want to backtrace injections which were done

Yes, better to change the wording then.

This comment has been minimized.

@markus2330

markus2330 Sep 30, 2018

Contributor

And why not add the changes directly to the keys where the changes occurred?

@markus2330

markus2330 Sep 30, 2018

Contributor

And why not add the changes directly to the keys where the changes occurred?

This comment has been minimized.

@Piankero

Piankero Oct 13, 2018

Contributor

And why not add the changes directly to the keys where the changes occurred?

Because some injections simply remove a whole configuration/ section. It is also much simpler to use when all changes can be found on one key's metadata than searching for all keys if they have an injection on them imo

@Piankero

Piankero Oct 13, 2018

Contributor

And why not add the changes directly to the keys where the changes occurred?

Because some injections simply remove a whole configuration/ section. It is also much simpler to use when all changes can be found on one key's metadata than searching for all keys if they have an injection on them imo

Piankero added some commits Sep 30, 2018

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Oct 14, 2018

Contributor

Please only set "ready to merge" if the PR compiles.

Contributor

markus2330 commented Oct 14, 2018

Please only set "ready to merge" if the PR compiles.

@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Oct 15, 2018

Contributor

This whole PR does only exist since you told me to do a PR for large README's so it is easier to comment on. You requested a change:

Yes, better to change the wording then.

which I did and as you again told me yourself I should mark the PR as "ready to merge" once you should look at it again. This PR wont get merged anyway so why do you require it to compile??

So what should I do now? Mark it/ write you an email/ label it with something otherwise/ just do an issue ...

Contributor

Piankero commented Oct 15, 2018

This whole PR does only exist since you told me to do a PR for large README's so it is easier to comment on. You requested a change:

Yes, better to change the wording then.

which I did and as you again told me yourself I should mark the PR as "ready to merge" once you should look at it again. This PR wont get merged anyway so why do you require it to compile??

So what should I do now? Mark it/ write you an email/ label it with something otherwise/ just do an issue ...

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Oct 15, 2018

Contributor

You just replaced log -> change, what should I review here? Please request reviews more carefully.

The logging strategy should have already been a part of the architecture, so it is now quite complicated to give input about the big picture.

@mpranj suggested that we should have information that is passed from plugins to plugins in a separate keyset, similar to the plugin handle and plugin config. Your "logs/changes" would be much better placed there.

Contributor

markus2330 commented Oct 15, 2018

You just replaced log -> change, what should I review here? Please request reviews more carefully.

The logging strategy should have already been a part of the architecture, so it is now quite complicated to give input about the big picture.

@mpranj suggested that we should have information that is passed from plugins to plugins in a separate keyset, similar to the plugin handle and plugin config. Your "logs/changes" would be much better placed there.

@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Oct 15, 2018

Contributor

You just replaced log -> change, what should I review here? Please request reviews more carefully.

The only "task" you explicitly gave me is to change the wording. I did that and now want to see if the whole concept is finished. How should I tell you to take a whole look on the concept other than labeling the PR?

@mpranj suggested that we should have information that is passed from plugins to plugins in a separate keyset, similar to the plugin handle and plugin config. Your "logs/changes" would be much better placed there.

Where did he suggest that?

Contributor

Piankero commented Oct 15, 2018

You just replaced log -> change, what should I review here? Please request reviews more carefully.

The only "task" you explicitly gave me is to change the wording. I did that and now want to see if the whole concept is finished. How should I tell you to take a whole look on the concept other than labeling the PR?

@mpranj suggested that we should have information that is passed from plugins to plugins in a separate keyset, similar to the plugin handle and plugin config. Your "logs/changes" would be much better placed there.

Where did he suggest that?

@mpranj

This comment has been minimized.

Show comment
Hide comment
@mpranj

mpranj Oct 15, 2018

Contributor

@Piankero we discussed it in the meeting last week, but I have not written anything about it in doc/decisions yet.

I needed some means of communication between plugins. I proposed to use a KeySet for my scenario and @markus2330 suggested that it can be useful for other plugins too. Plugins would have a handle to a "global" KeySet with the sole purpose of communication between plugins.

Contributor

mpranj commented Oct 15, 2018

@Piankero we discussed it in the meeting last week, but I have not written anything about it in doc/decisions yet.

I needed some means of communication between plugins. I proposed to use a KeySet for my scenario and @markus2330 suggested that it can be useful for other plugins too. Plugins would have a handle to a "global" KeySet with the sole purpose of communication between plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment