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

Support remembering previously completed Verb Installations #2070

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

madoar
Copy link
Collaborator

@madoar madoar commented Aug 24, 2019

This PR does the Phoenicis part discussed in #1824.

I'm not sure yet whether we want to add the new methods registerVerb and isVerbRegistered to the engine class, because they seem somehow unrelated to the engine to me.

madoar added 3 commits August 24, 2019 15:30
- add a label that informs the user if a verb is already installed
- add a method to the VerbsManager to check whether a verb has already been installed
@plata
Copy link
Collaborator

plata commented Aug 25, 2019

I agree. A Container class makes more sense.

@madoar
Copy link
Collaborator Author

madoar commented Aug 25, 2019

I thought about adding a new script to Utils maybe named container_utils. This script would export the above mentioned two methods. This has the benefit that the behavior is independent of the engine and it is located in a fixed location (under a fixed script id).

What do you think?

@plata
Copy link
Collaborator

plata commented Aug 26, 2019

Why should it be a script and not in Java?

@madoar
Copy link
Collaborator Author

madoar commented Aug 26, 2019

That is a good point. It doesn't need to be a script it could also be implemented in Java. For me it is more intuitive to implement it in Java because we will access the registerVerb method from inside the scripts and not from Java.

@plata
Copy link
Collaborator

plata commented Aug 27, 2019

For me it is more intuitive to implement it in Java because we will access the registerVerb method from inside the scripts and not from Java.

Did you really mean "implement it in Java" or "implement it in Javascript"?

@madoar
Copy link
Collaborator Author

madoar commented Aug 27, 2019

implement it in JavaScript

@plata
Copy link
Collaborator

plata commented Aug 28, 2019

Ok, then the sentence makes more sense. Nevertheless, for me this should be in Java because it's nothing you would want to influence with custom scripts (it's similar to e.g. ShortcutManager).

@madoar
Copy link
Collaborator Author

madoar commented Aug 28, 2019

So basically you would add the two methods to a new with @Safe annotated Java class?

I.e.:

@Safe
public class VerbUtils {
   void registerVerb(String verbId) {
      ...
   }

   boolean isVerbRegistered(String verbId) {
      ...
   }
}

@plata
Copy link
Collaborator

plata commented Aug 29, 2019

Yes, something like that. We can decide if it belongs more to a verb or to a container.

@madoar
Copy link
Collaborator Author

madoar commented Aug 31, 2019

I have some problems getting the container information based on its name/id...

In a nutshell I want to add the following class to phoenicis-tools:

package org.phoenicis.tools.container;

import org.phoenicis.configuration.security.Safe;

@Safe
public class ContainerUtils {
    void registerVerb(String containerId, String verbId) {
        ...
    }

    boolean isVerbRegistered(String containerId, String verbId) {
        ...
    }
}

The first thing I need to do for both methods is fetching the content of the phoenicis.cfg file from the container, but how do I do that? We also don't seem to have the ContainerDTO class available in phoenicis-tools.

@plata
Copy link
Collaborator

plata commented Sep 1, 2019

Why don't you put it to phoenicis-containers?

@madoar
Copy link
Collaborator Author

madoar commented Sep 1, 2019

Do we have @safe accessible from pheonicis-containers? Can we make the class accessible from the scripts if we put it in phoenicis-containers?

@plata
Copy link
Collaborator

plata commented Sep 1, 2019

From my understanding, @qparis intention was to always put the JS API into the package where it belongs logically (and not all API together in one place). So yes, this should be possible.

@madoar
Copy link
Collaborator Author

madoar commented Sep 1, 2019

I think that it may be a good idea to wait with this PR until we have changed the repository structure to a more flexible one (I can't find the corresponding issue). Afterwards I think it will also be easier to fetch an entity (i.e. application, script, container etc.) for a given id/name. This should solve a lot of the problems I have in this PR.

@plata
Copy link
Collaborator

plata commented Sep 2, 2019

Ok.

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