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

Add oslc-ui-model module #69

Merged
merged 5 commits into from
Jun 10, 2021
Merged

Add oslc-ui-model module #69

merged 5 commits into from
Jun 10, 2021

Conversation

berezovskyi
Copy link
Member

@berezovskyi berezovskyi commented Dec 18, 2020

Description

Package Java classes from https://github.com/eclipse/lyo.oslc-ui/runs/1574315947 as a org.eclipse.lyo.domains:ui-classes module in the main repo.

Checklist

  • This PR adds an entry to the CHANGELOG. See https://keepachangelog.com/en/1.0.0/ for instructions. Minor edits are exempt.
  • This PR was tested on at least one Lyo OSLC server or adds unit/integration tests.
  • This PR does NOT break the API

Issues

See eclipse/lyo.oslc-ui#3

@berezovskyi
Copy link
Member Author

@jadelkhoury @AndreyBespamyatnov please review

@berezovskyi berezovskyi force-pushed the b-oslc-ui-model branch 2 times, most recently from 2008523 to e6e509a Compare June 6, 2021 00:07
@berezovskyi
Copy link
Member Author

@jadelkhoury please check if this is suitable for your use in the LyoD Generator. I am ready to merge and release this in 4.1.0.alpha.

@jadelkhoury
Copy link
Contributor

@berezovskyi ! So are these files actually copied from a build from
https://github.com/eclipse/lyo.oslc-ui/blob/master/package.json?
When is this triggered? when a change is made in oslc-ui or core?

Finally, you are using "domains/ui-classes". (a) Why under "domains"? (b) rename ui-classes to "oslc-ui"?

@berezovskyi
Copy link
Member Author

berezovskyi commented Jun 7, 2021

So are these files actually copied from a build from
https://github.com/eclipse/lyo.oslc-ui/blob/master/package.json

Yes

When is this triggered? when a change is made in oslc-ui or core?

By hand. Edit: I did not see any template placeholders in the code. It will be best to move the classes here and update the instructions in the original repo to add this new Maven module as a dependency.

Finally, you are using "domains/ui-classes". (a) Why under "domains"? (b) rename ui-classes to "oslc-ui"?

These are just model classes, not the actual OSLC UI, I am happy to rename but confusion must be avoided. Please suggest a better name but not oslc-ui. How about oslc-ui-model?

@jadelkhoury
Copy link
Contributor

So are these files actually copied from a build from
https://github.com/eclipse/lyo.oslc-ui/blob/master/package.json

Yes

When is this triggered? when a change is made in oslc-ui or core?

By hand. Edit: I did not see any template placeholders in the code. It will be best to move the classes here and update the instructions in the original repo to add this new Maven module as a dependency.

It would be great of course if we can move them here. But I think they are being build from this, lines 10-13.
https://github.com/eclipse/lyo.oslc-ui/blob/master/package.json

Finally, you are using "domains/ui-classes". (a) Why under "domains"? (b) rename ui-classes to "oslc-ui"?

These are just model classes, not the actual OSLC UI, I am happy to rename but confusion must be avoided. Please suggest a better name but not oslc-ui. How about oslc-ui-model?

I mean,

  1. instead of being under "lyo\domains\ui-classes", can we put them under "lyo\ui-classes".
    Why are they under "domains"? Seems odd.
  2. If we agree on (1), the next step is that the project/folder needs a name that makes sense. anything with oslc-ui makes sense to me.

@berezovskyi
Copy link
Member Author

But I think they are being build from this

You are right, I forgot about that.

instead of being under "lyo\domains\ui-classes

How about server/oslc-ui-model?

@berezovskyi
Copy link
Member Author

Alternatively, @jadelkhoury, we can use https://github.com/joelittlejohn/jsonschema2pojo#jsonschema2pojo-- native Maven generation capabilities. The problem is that the JSON file is generated from JavaScript (TypeScript)...

@jadelkhoury
Copy link
Contributor

Naming server/oslc-ui-model is fine.

How do we proceed with the build? Could we not build the stuff from https://github.com/eclipse/lyo.oslc-ui?
We do eventually also want to build (& release) the javascript stuff.

I was thinking that the build can create a "build" folder under lyo.oslc-ui.
the this lyo build can copy the classes from there!

@berezovskyi
Copy link
Member Author

berezovskyi commented Jun 7, 2021

Could we not build the stuff from eclipse/lyo.oslc-ui?

Actually, we can and it may be the best solution. I was reluctant to suggest this option as it would require to redo all the deployment setup to Eclipse Maven and OSSRH repositories. Let's make the move when there will be changes to the schema so that we don't end up doing extra work for something that won't change much?

@berezovskyi
Copy link
Member Author

berezovskyi commented Jun 7, 2021

I was thinking that the build can create a "build" folder under lyo.oslc-ui.
the this lyo build can copy the classes from there!

This will require us to pull another repo during the build of this repo and it'd rather avoid it.

Naming server/oslc-ui-model is fine.

I will update the PR shortly.

@jadelkhoury
Copy link
Contributor

ok. go ahead.

But just to be sure, it is possible to build the code on oslc-ui?

@berezovskyi
Copy link
Member Author

it is possible to build the code on oslc-ui?

Sure, but it's not of much use unless we publish JARs to Eclipse Maven and Maven Central (via OSSRH in the middle).

@jadelkhoury
Copy link
Contributor

This is what I do to include swagger-ui code into an oslc server.

https://github.com/OSLC/lyo-adaptor-sample-modelling/blob/b43870fc01c7b28bf130861287f295ab21b4ba67/adaptor-rm-webapp/pom.xml#L186

I guess you mean the same, right?

@berezovskyi
Copy link
Member Author

No, I think we mean different things. Your example uses a Maven package to include resources other than Java classes in the project.

This particular module is code-only. The npm package that is in the other repo could be converted to (or rather augmented by) a Maven module that will contain no code and just the generated resources that can be copied into the project using the approach you linked to. Otherwise that npm package should be published on npm, e.g. https://www.npmjs.com/package/oslc-service (just deployed it for the test).

@berezovskyi
Copy link
Member Author

@jadelkhoury please check

@jadelkhoury
Copy link
Contributor

Looks good, except that I have since then updated the PreviewFactory class. So, this is a good test on how/when things get updated.

I also get the impression that "Server" is where we are putting old obsolete stuff (wink, oauth). But logically, I don't see a problem with the location.

@berezovskyi
Copy link
Member Author

berezovskyi commented Jun 8, 2021 via email

Signed-off-by: Andrew Berezovskyi <andrew@berezovskyi.me>
@berezovskyi
Copy link
Member Author

@jadelkhoury I just ported your commit to this branch. I will update the oslc-ui instructions once this PR is merged and we confirm the module can be loaded as a dependency.

@jadelkhoury
Copy link
Contributor

@jadelkhoury I just ported your commit to this branch. I will update the oslc-ui instructions once this PR is merged and we confirm the module can be loaded as a dependency.

How about we merge, and then I can test and update the instructions accordingly? I think it is good to go.

@berezovskyi berezovskyi changed the title Add ui-classes module Add oslc-ui-model module Jun 10, 2021
@berezovskyi berezovskyi merged commit 08aa6af into master Jun 10, 2021
@berezovskyi berezovskyi deleted the b-oslc-ui-model branch June 10, 2021 08:52
@berezovskyi
Copy link
Member Author

Added instructions in eclipse/lyo.oslc-ui#8, ready to test once https://ci.eclipse.org/lyo/job/lyo-monorepo/79/ succeeds.

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.

None yet

3 participants