Skip to content

Added commands to install&update devon4node and cicdgen#280

Merged
hohwille merged 9 commits intodevonfw:masterfrom
dario-rodriguez:feature/devon4node-cicdgen
Oct 29, 2019
Merged

Added commands to install&update devon4node and cicdgen#280
hohwille merged 9 commits intodevonfw:masterfrom
dario-rodriguez:feature/devon4node-cicdgen

Conversation

@dario-rodriguez
Copy link
Copy Markdown
Member

This PR includes two new commands:

  • devon4node
  • cicdgen

The purpose of those commands is to install&update&execute those devonfw tools.

@hohwille
Copy link
Copy Markdown
Member

hohwille commented Oct 21, 2019

@dario-rodriguez thanks for this PR. This is very valuable stuff. 👍
Actually I was on a business trip hanging around at airports the day this PR was submitted and we were finalizing the release as we reached code-freeze for devonfw 3.2.0.
If you want to have this feature included into 3.2.0 we would need to discuss with @sjimenez77 about this. However, my suggestion would be to post pone this to a next release. We can even release 3.2.x with this new feature even though it might not be exactly aligned with semantic versioning but nothing should prevent us from shipping new valuable features once they are ready to be published.

I have not tested your changes but as cicdgen is already out in the wild, I assume this feature would be ready to go and besides someone else testing it, I would consider this as accepted already.
Regarding devon4node we should discuss as this does not seem to fit in the current philosophy of devon-ide:
We already have named the main CLI command devon. For that reason the CLI is designed to be DRY (do not repeat yourself). This means that we already have commands like
devon java create ... to create a java project and are not naming this devon devon4j create ....
In other words that means that I want to propose that you would integrate these features directly into the node commandlet that already exists. This also reduces dependencies as in order to run anything with the devon4node commandlet, you would most probably have node (node.js) to be installed as a prerequesite. See #168

Does that make sense to you?
In case you want to speed up cicdgen integration, you can even split up this PR into two PRs so we can merge individually and discussions on the one does not block the other.

@dario-rodriguez
Copy link
Copy Markdown
Member Author

Hello,

I created this PR because Santos wants it in the next release.
When I created this new commands, I follow the existing one for ng so it have as prerequisite to have installed npm. I'm my opinion if you want to change this for devo4node you must do the same for devon4ng.
Related to the naming, as we already have a command named node I could call it node. Im open to suggestions.

@dario-rodriguez
Copy link
Copy Markdown
Member Author

Anyway, for me doesn't makes sense create a command inside the ide in order to execute devon4node commands. For me is ok if you install it globally when you install npm.

@sjimenez77
Copy link
Copy Markdown
Member

While we have access to devon4node and cicdgen commands globally once devonfw ide is installed for me it si completely fine too. Both are CLI tools and for the moment it is not necessary to anything else.

Does it make sense?

@hohwille
Copy link
Copy Markdown
Member

I think it is complex to clarify this while chatting in this issue. I was asking for workshops and discussions about shaping the devon-ide CLI and its philosophy. So far nobody ever gave feedback to any of my questions regarding the CLI design so I made it up on my own.
I will try to setup a virtual call so we can have a discussion and everybody understands the whole story (as I think this is not yet clear). As everybody is very busy and we are also sharp on time with the release we need some kind of luck to get everything together...

@hohwille
Copy link
Copy Markdown
Member

So devon4node is an npm module that has already been published:
https://www.npmjs.com/package/@devon4node/devon4node?activeTab=readme
That page says that the source-code can be found here:
https://github.com/devonfw/devon4node
However, I can not find it there. This is already something we should improve on.

Further devon4node is the name of a Stack in devonfw while it now seems also to be the name of a tool that however is something different though it is obviously is a tool that supports using the devon4node stack, so I get why it was named like this.
Do not get me wrong. I do not want to force you to rewrite this tool in bash or so, I am just trying to fight for some kind of consistency for devonfw and the CLI of devon-ide. I also got requests to change names/repos e.g. in devon-ide and sonar-devon4j-plugin. In the first place this seems like annyoing pain but I fully agree that we want to have some consistency within devonfw and in the products we provide.
Can't we just put the logic to download this tool into the existing node commandlet? Maybe we could even just delegate from the commandlet to devon4node so that we have a consistent CLI UX? So e.g. devon node generate «args» would do install devon4node if not installed already and then delegate to devon4node generate «args». Also devon node create «args» would do the same with devon4node new «args» to be consistent with devon java create «args» or devon ng create «args» that all already exists. Otherwise we will create an inconsistent zoo of CLI commands that will confuse the user. If you do not like the current decision of the devon-ide CLI, I am also open to discuss and change (before the official release is published) - however, be ware that we then wasted quite some time but if it will lead to better UX I am also up to that.
If the user wants to use devon4node directly he still can do so and if he prefers to have devon as a single CLI entry point he is also fine.

@dario-rodriguez
Copy link
Copy Markdown
Member Author

Hello, as you can see in the npm page, this a beta version so the code is not merge yet. You can find it in the PR is already created: devonfw/devon4ts#17
Related with the name, we already noted that name is a bit confusing, so we decided to change the name to @devon4node/cli. The problem is that some ppl are testing the current one so we won't release a new version until we have the feedback.
Maybe the name confuses you so let me explain what we want: Add to devon-ide only the denvon4node CLI, like cobigen and ng.
The motivation is, as you mention before, to be align with other stacks. As the command node already exists maybe I choose a bad name for the new one.
I will update the node command in order to follow your comments and then we can discuss if this approach match with the ide.

@maybeec
Copy link
Copy Markdown
Member

maybeec commented Oct 22, 2019

Hi @dario-rodriguez, what is quite interesting is, that you are able to generate also devon4j archetype with your cli. Is that right? What is the difference to the command devon4j provided into the devon ide, which generates a new devon4j project?

In addition, I would suggest to include the commands of the devon4ng cli into the respective commands already available. For example as @hohwille said, there is already the command devon java create ... which creates a new devon4j project based on the devon4j maintained archetype. If devon4ng cli does something different it might be interesting for consistency to include the devon4j generation of devon4ng cli into the devon java ... command.

@dario-rodriguez
Copy link
Copy Markdown
Member Author

Hello @maybeec ,
The two commands that I added to this PR are:

  • devon4node (needs rework): this command create a new devon4node projects.
  • cicdgen: this command is for generate all files related to CICD (pipelines, Dockerfiles...) for all devonfw stacks. It's in the context of devonfw shop floor and prepare the project to work in the Production Line and the deployment with docker/OpenShift.

Another solution could be, instead of creating a new command for cicdgen, add it in the current commands (java, ng, node) like: devon java cicd, devon ng cicd or something similar. Anyway, it must be separated from the current create command, as not all project that uses devon4j will use the PL or docker. That's why we didn't add those files to the current archtype.

What do you think @hohwille ? In your opinion which is the best approach?

@maybeec
Copy link
Copy Markdown
Member

maybeec commented Oct 22, 2019

This really sounds good. I like the idea of the sub "cicd" command.

@dario-rodriguez
Copy link
Copy Markdown
Member Author

I updated this PR with the latest requirements.

Copy link
Copy Markdown
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dario-rodriguez Awesome. Looking very good now 👍
IMHO the PR could already be merged. However, I added some review comments and ask you to have a look and see if you can do some improvements. If we do not have any time left, we could merge as is and rework later. However, I think it should be easy to rework the last improvements.
Thank you so much for your valuable contribution.

Comment thread scripts/src/main/resources/scripts/command/node
Comment thread documentation/cicdgen.asciidoc
Comment thread scripts/src/main/resources/scripts/command/node Outdated
Comment thread scripts/src/main/resources/scripts/command/node
Comment thread scripts/src/main/resources/scripts/command/node Outdated
@dario-rodriguez
Copy link
Copy Markdown
Member Author

Hello @hohwille ,

This PR is updated in order to satisfy your comments.

Copy link
Copy Markdown
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dario-rodriguez very well done. Awesome now. Thank you so much 👍

@hohwille hohwille merged commit 5793fd2 into devonfw:master Oct 29, 2019
@hohwille hohwille added this to the release:3.2.0 milestone Oct 29, 2019
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.

4 participants