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

Packaging restx archives #72

Merged
merged 5 commits into from Apr 1, 2014
Merged

Packaging restx archives #72

merged 5 commits into from Apr 1, 2014

Conversation

@fcamblor
Copy link
Contributor

fcamblor commented Mar 18, 2014

This is a follow up for #62
I provided a new app archive command, allowing to generate a restx archive.

Some insight about the restx archive : the hello-world application generated from a restx app new will generate something like this :

$> tree
.
├── META-INF
│   ├── restx
│   │   └── app
│   │       ├── data
│   │       │   ├── credentials.json
│   │       │   └── users.json
│   │       ├── md.restx.json
│   │       ├── module.ivy
│   │       ├── pom.xml
│   │       └── src
│   │           ├── main
│   │           │   ├── java
│   │           │   │   └── helloworld
│   │           │   │       ├── AppModule.java
│   │           │   │       ├── AppServer.java
│   │           │   │       ├── Roles.java
│   │           │   │       ├── domain
│   │           │   │       │   └── Message.java
│   │           │   │       └── rest
│   │           │   │           └── HelloResource.java
│   │           │   ├── resources
│   │           │   │   └── logback.xml
│   │           │   └── webapp
│   │           │       └── WEB-INF
│   │           │           └── web.xml
│   │           └── test
│   │               ├── java
│   │               │   └── helloworld
│   │               │       └── rest
│   │               │           └── HelloResourceSpecTest.java
│   │               └── resources
│   │                   └── specs
│   │                       └── hello
│   │                           ├── should_admin_say_hello.spec.yaml
│   │                           ├── should_user1_say_hello.spec.yaml
│   │                           └── should_user2_not_say_hello.spec.yaml
│   └── services
│       └── restx.factory.FactoryMachine
├── helloworld
│   ├── AppModule.class
│   ├── AppModuleFactoryMachine$1.class
│   ├── AppModuleFactoryMachine$2.class
│   ├── AppModuleFactoryMachine$3.class
│   ├── AppModuleFactoryMachine$4.class
│   ├── AppModuleFactoryMachine$5.class
│   ├── AppModuleFactoryMachine.class
│   ├── AppServer.class
│   ├── Roles.class
│   ├── domain
│   │   └── Message.class
│   └── rest
│       ├── HelloResource.class
│       ├── HelloResourceFactoryMachine$1.class
│       ├── HelloResourceFactoryMachine.class
│       ├── HelloResourceRouter$1.class
│       ├── HelloResourceRouter.class
│       ├── HelloResourceRouterFactoryMachine$1.class
│       └── HelloResourceRouterFactoryMachine.class
└── logback.xml

ATM, I'm packaging sources files into the restx archive (because api doc might rely on it to display source contents)
For the same reason, I package the src/test/* file hierarchy.

You can test it easily :

restx app new
................
restx app archive /tmp/hello.jar
................
restx app grab file:///tmp/hello.jar + app run

FYI, I just created a maven-restx-plugin which will re-use the RestxArchive.pack() utility method, in order to package a standard restx archive (with -restx classifier).

Important note : having the *.class files in the archive is totally useless since the first thing I do in app grab is to unzip & chroot to META-INF/restx/app/ directory (then, auto compile does the stuff for us).

@fcamblor fcamblor changed the title App archive Packaging restx archives Mar 18, 2014
@fcamblor
Copy link
Contributor Author

fcamblor commented Mar 18, 2014

It would be a good thing to extract RestxArchive into a dedicated module (since the restx-maven-plugin relies on it, and some other tools like ant or gradle might want to rely on it in order to provide a smooth integration for generating restx archives)

But I don't know where to put it.

I don't think putting it inside restx-common would be a good idea since it would be really uncommon to have to manipulate RestxArchive when developping with restx.
OTOH, creating a dedicated module for only 1 class seems weird :P

@fcamblor

This comment has been minimized.

I would need some help on this : is it easily feasible ? (namely ivy.xml / pom.xml => md.restx.json file)

This comment has been minimized.

Copy link

xhanin replied Mar 18, 2014

This comment has been minimized.

Copy link
Owner Author

fcamblor replied Mar 30, 2014

Implemented it on 6da7c1a & 4b26b5b

@xhanin
Copy link
Contributor

xhanin commented Mar 18, 2014

Good job! I've reviewed the commits, everything seems fine to me.

About what is packaged inside the app, maybe making that configurable would be nice. Maybe something very simple like making the list Arrays.asList("target", "tmp", "logs") configurable through a system property or an injected Named String.

@fcamblor
Copy link
Contributor Author

fcamblor commented Mar 21, 2014

I'm ok with an optional command parameter such as restx app archive /tmp/foo.jar -excludes "target,tmp,logs"

About injected named string, I suppose you're talking about an entry in the AppSettings class ? Named, for instance, restx.archive.excludes ?
How will the overloading of these properties be made at application level ?
By providing a Name(ConfigSupplier.class, "coreAppConfigSupplier") with overriden restx.archive.exclude property in the application's AppModule ? (because re-defining an appConfig.properties at application level is dangerous, especially on restx version upgrades)
Is there an easier way ?

@xhanin
Copy link
Contributor

xhanin commented Mar 22, 2014

Yes, something in the AppSettings. Then to override it you only need to
provide a @nAmed String (either through settings, system property or
@provides) with better priority (i.e. lower priority number), you don't
need to override the whole config supplier.

On Fri, Mar 21, 2014 at 10:31 AM, Frédéric Camblor <notifications@github.com

wrote:

I'm ok with an optional command parameter such as restx app archive
/tmp/foo.jar -excludes "target,tmp,logs"

About injected named string, I suppose you're talking about an entry in
the AppSettings class ? Named, for instance, restx.archive.excludes ?
How will the overloading of these properties be made at application level ?
By providing a Name(ConfigSupplier.class, "coreAppConfigSupplier") with
overriden restx.archive.exclude property in the application's AppModule ?
(because re-defining an appConfig.properties at application level is
dangerous, especially on restx version upgrades)
Is there an easier way ?

Reply to this email directly or view it on GitHubhttps://github.com//pull/72#issuecomment-38260081
.

Xavier Hanin
Lead architect at 4SH France - http://www.4sh.fr/
RESTX creator and lead developer - http://restx.io/
BordeauxJUG creator - http://www.bordeauxjug.org/
Apache Ivy Creator - http://ant.apache.org/ivy/

@fcamblor
Copy link
Contributor Author

fcamblor commented Mar 29, 2014

Back on the topic.

Ok then if I add an entry in the AppSettings, how would I get "current application's AppSettings instance" from the restx-shell classloader ?

Would there be a shell mechanism allowing to provide current visited app classes in a dedicated classloader, thus allowing to create a Factory from current visited application classes (where will reside the AppSettings) ?

@xhanin
Copy link
Contributor

xhanin commented Mar 29, 2014

That sounds like too much complexity for such little value. A better place
may be a property in md.restx.json, or simply don't support it except
through command parameters after all, we don't even know if someone will
actually have to change these excludes :).

On Sat, Mar 29, 2014 at 4:00 PM, Frédéric Camblor
notifications@github.comwrote:

Back on the topic.

Ok then if I add an entry in the AppSettings, how would I get "current
application's AppSettings instance" from the restx-shell classloader ?

Would there be a shell mechanism allowing to provide current visited app
classes in a dedicated classloader, thus allowing to create a Factoryfrom current visited application classes (where will reside the
AppSettings) ?

Reply to this email directly or view it on GitHubhttps://github.com//pull/72#issuecomment-38997965
.

Xavier Hanin
Lead architect at 4SH France - http://www.4sh.fr/
RESTX creator and lead developer - http://restx.io/
BordeauxJUG creator - http://www.bordeauxjug.org/
Apache Ivy Creator - http://ant.apache.org/ivy/

@fcamblor
Copy link
Contributor Author

fcamblor commented Mar 29, 2014

Note that currently, in different shell commands, we rely on AppSettings for some configuration values.
But we are, I think, fooled by the different contexts (shell context and current application context) in most of the cases.

Maybe should we file an issue about this topic ?

fcamblor added 2 commits Mar 29, 2014
allowing to retrieve supported foreign (aka non restx) module descriptors
in a given directory
…resent

Because without these descriptor, the restx archive won't be a real restx archive
(auto download of dependencies won't be possible during `restx app grab + app run`)
@xhanin
Copy link
Contributor

xhanin commented Apr 1, 2014

About the usage of AppSettings, yes an issue to address it would be fine. I think at least some of these settings should be made available to the shell easily (at least all the paths and app package), and therefore having to rely on the module sources / classpath is not very simple / easy to address. But I currently don't see how to make that simple, so an issue to later address it is fine for me.

@xhanin
Copy link
Contributor

xhanin commented Apr 1, 2014

And BTW the PR is fine for me like that, feel free to merge it, it's a nice feature!

fcamblor added a commit that referenced this pull request Apr 1, 2014
@fcamblor fcamblor merged commit c33a444 into restx:master Apr 1, 2014
@fcamblor
Copy link
Contributor Author

fcamblor commented Apr 8, 2014

Concerning RestxArchive location, I'm wondering if it wouldn't be better to move the RestxArchive class to restx-build module.

It would be more natural for external tools to depend on restx-build than on restx-core-shell.

Drawback for this will be to add a dependency restx-build -> restx-core in order to be able to manipulate AppSettings class (plus some guava / commons-io classes) inside RestxArchive

WDYT ? (if you're ok, I'll move the class before the release)

@xhanin
Copy link
Contributor

xhanin commented Apr 8, 2014

I agree that its current location doesn't make it very easy to reuse, and that moving it to restx-build is probably a good idea. But I think it's a pretty big change in terms of module dependencies (especially with the requirement to have the core module as a dependency, I'm not sure we shouldn't rather move AppSettings to another module / review it / split it / discuss it). So I wouldn't change that the day of the release, I think it can wait.

@fcamblor
Copy link
Contributor Author

fcamblor commented Apr 8, 2014

ok I'm fine with it.

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.