search

jhipster / jhipster-core

JHipster Domain Language, used by JHipster UML and JDL-Studio to generate entities
Apache License 2.0
346 stars 116 forks source link

Add blueprints #344

Closed MathieuAA closed 4 years ago

MathieuAA commented 5 years ago

Please make sure the below checklist is followed for Pull Requests.

deepu105 commented 5 years ago

I'm not in favour of doing any version management for blueprints. We should let the user manage it using NPM/yarn. We just use what is installed in current project or globally IMO

On Sat, 6 Jul 2019, 11:50 am Aurélien Mino, notifications@github.com wrote:

@murdos commented on this pull request.

In lib/exporters/jhipster_application_exporter.js https://github.com/jhipster/jhipster-core/pull/344#discussion_r300826550 :

@@ -79,6 +80,7 @@ function setUpApplicationStructure(application) { }

function setUpArrayOptions(application) {

  • application[GENERATOR_NAME].blueprints = Array.from(application[GENERATOR_NAME].blueprints);

Another way could be joining them in a single string, e.g blueprint@version(although we should take care for scoped blueprints as well. Any other ideas?

This is something I already thought about, but:

  1. It means we should also support it from CLI : --blueprints kotlin@1.0.0,vuejs@1.1.0
  2. It implies that generator-jhipster should install beforehand the corresponding npm packages, which is something we have never done. During the initial generator, the generator is just saving versions used for the generator.

I think it could be an interesting feature to have, but that implies a few changes in the current behavior of the generator.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jhipster/jhipster-core/pull/344?email_source=notifications&email_token=AAIOKFZNH4D5ULOMH3IUJ2LP6BTFXA5CNFSM4H6JK4J2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB5U24TY#discussion_r300826550, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIOKF7OTSXINTZ5FD5SU43P6BTFXANCNFSM4H6JK4JQ .

pvliss commented 5 years ago

It means we should also support it from CLI : --blueprints kotlin@1.0.0,vuejs@1.1.0

There is actually already some code to achieve that(see here ) and is used by the upgrade sub-gen to support the --targetBlueprintVersions flag.

I'm not in favour of doing any version management for blueprints. We should let the user manage it using NPM/yarn. We just use what is installed in current project or globally IMO

I agree that we should keep things as simple as possible and this is the current behavior but does installing blueprints on first generation counts for version managment? I consider it simply as a convenience and since the upgrade sub-gen does in fact installs user specified or latest version then we could at least be consistent. It is still up to the user to specify a version or just use the latest.

From what I've seen the blueprint version is used in the follownig cases:

Other non-obvious usages are:

Still I think that blueprint versions should not be specified in the JDL and we can also remove the version: latest default value from the main generator as well and only save the actual one used when the project was generated as meta data for the use cases described above.

murdos commented 5 years ago

I'm not in favour of doing any version management for blueprints. We should let the user manage it using NPM/yarn. We just use what is installed in current project or globally IMO

Yeah, that sounds much simpler and aligned with the current usage.

From what I've seen the blueprint version is used in the follownig cases:

  • Initial generation commit message.
  • By the upgrade sub-gen to check for new version and in the commit message as well

The primary usage is to be able to add the blueprint as dev dependency, so things work when you try to use e.g. the entity subgen from a different user/workstation than the one used to generate the application. It's then also a mandatory information for the upgrade sub-gen.

Still I think that blueprint versions should not be specified in the JDL and we can also remove the version: latest default value from the main generator as well and only save the actual one used when the project was generated as meta data for the use cases described above.

I agree.

I'm however not sure to understand what you mean exactly about removing "version: latest default value from the main generator". When I've introduced blueprintVersion, I tried to handle existing applications that were missing this piece of information.

pvliss commented 5 years ago

I'm however not sure to understand what you mean exactly about removing "version: latest default value from the main generator"

I have just added it as the default in the created object to avoid having to check in several locations, but we can definitely remove that and keep only actual versions upon initial generation.

So how about the following:

WDYT?

deepu105 commented 5 years ago

Lets stick with just simple blueprint names. Let's not save any version info and let user handle it. Its what we do with the generator. During upgrade we only handle generator and not blueprint versions for now

On Sat, 6 Jul 2019, 8:23 pm Panayiotis Vlissidis, notifications@github.com wrote:

I'm however not sure to understand what you mean exactly about removing "version: latest default value from the main generator"

I have just added it as the default in the created object to avoid having to check in several locations, but we can definitely remove that and keep only actual versions upon initial generation.

So how about the following:

"blueprints": [ "vuejs", "kotlin" ]

  • Keep the --blueprints option flag as it is used now(i.e comma separated list without version) e.g. --blueprints vuejs, kotlin and do not install any but expect them to be already installed.
  • Change the main generator to read the config as an array of strings and have the ability to also recognize the versions and of-course append them when actual generation takes place. So for example reading the above example generated by the JDL it would finally save in the .yo-rc.json the following:

"blueprints": [ "generator-jhipster-vuejs@1.1.0", "generator-jhipster-kotlin@0.8.1" ]

Of-course this will need some follow up changes in the main generator in order to work.

WDYT?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jhipster/jhipster-core/pull/344?email_source=notifications&email_token=AAIOKFYJCSAEMB42Q6APMVLP6DPLJA5CNFSM4H6JK4J2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZK6GZQ#issuecomment-508945254, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIOKF2YULZPVN5NEEST5T3P6DPLJANCNFSM4H6JK4JQ .

murdos commented 5 years ago

Lets stick with just simple blueprint names. Let's not save any version info and let user handle it. Its what we do with the generator.

I disagree. Saving the version of the blueprint(s) (in one way or another) that has been used to generate an application is as important as saving the jhipster version that has been used. Indeed this is the combination of both pieces of information that allow you regenerate your application in the exact same way, and thus use also use subgen such as entity, ci-cd, ... in the correct environment.

During upgrade we only handle generator and not blueprint versions for now

The current upgrade subgen already handle blueprint version. Not handling it anymore will both be a regression, and will break the upgrade subgen since the blueprint version is mandatory to correctly upgrade an application.

deepu105 commented 5 years ago

If you save the version to package.json when its generated it should take care of all that. Like how its handled for generator now. My point was not to save or manage version in our yo-rc or other config files

Deepu - Chat @ Spike [28znh]

On July 6, 2019 at 23:30 GMT, Aurélien Mino notifications@github.com wrote:

Lets stick with just simple blueprint names. Let's not save any version info and let user handle it. Its what we do with the generator.

I disagree. Saving the version of the blueprint(s) (in one way or another) that has been used to generate an application is as important as saving the jhipster version that has been used. Indeed this is the combination of both pieces of information that allow you regenerate your application in the exact same way, and thus use also use subgen such as entity, ci-cd, ... in the correct environment.

During upgrade we only handle generator and not blueprint versions for now

The current upgrade subgen already handle blueprint version. Not handling it anymore will both be a regression, and will break the upgrade subgen since the blueprint version is mandatory to correctly upgrade an application.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

pvliss commented 5 years ago

Have we reached a consensus here?

I guess @deepu105's solution to read the blueprint versions from package.json can also work for upgrades. If we are going to remove blueprint versions and rely solely on package.json then we will need to change the current way things are done as https://github.com/jhipster/generator-jhipster/pull/9937 has been merged. I can submit a follow up PR or we could revert it until we reach a final decision on how to handle this?

MathieuAA commented 5 years ago

What's left to do here? I'd like to get it merged before the end of the week :)

pvliss commented 5 years ago

We need to make a decission if versions of blueprints will be stored within yo-rc.json file or not. If yes then things will remain as is and I guess this is ok to merge. If not then will need to make a follow up PR to jhipster/generator-jhipster#9937 and you will need to revert https://github.com/jhipster/jhipster-core/commit/5ad7f188e4903def5738090ab459691ac7554b03.

MathieuAA commented 5 years ago

Okay, I'm in favor of having versions in the package.json file instead of the yo-rc.json file. I'm reverting my commit.

pvliss commented 5 years ago

OK. Will have a look to change the generator code as well

MathieuAA commented 5 years ago

@pvliss Hi! do you think this can be merged? I want to release a new (major) version soon and I'd like to ship this one too

pvliss commented 5 years ago

@MathieuAA Hello! I am sorry but I did not have the time to deal with this and most likely wont have the time for the weeks to come. I started some work at https://github.com/pvliss/generator-jhipster/tree/remove-blueprints-versions but never got to finish it or test it, hence it will not currrently work as is. So if you or @deepu105 believe that versions should be definitely removed then I would suggest to not merge this as then we will also need to change the generator as well. @jhipster/developers If someone else has more time and is interested then by all means please take over. If you want to merge it then you will need to put back c517a99ce5f82d00664070a8e0589b7437dbe9d2 that laso includes versione before doing so.

MathieuAA commented 4 years ago

@pvliss thanks for the heads up! I'm ok if we wait a bit for this one.

MathieuAA commented 4 years ago

Closing this, as it's too outdated. The code will almost be the same though.