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

#26: Implement ToolCommandlet for Jasypt #243

Merged

Conversation

mvomiero
Copy link
Contributor

@mvomiero mvomiero commented Mar 6, 2024

Closes #26

@coveralls
Copy link
Collaborator

coveralls commented Mar 6, 2024

Pull Request Test Coverage Report for Build 8380073666

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.5%) to 59.031%

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/common/Tag.java 2 96.24%
com/devonfw/tools/ide/commandlet/CommandletManagerImpl.java 3 88.89%
Totals Coverage Status
Change from base Build 8380056164: 0.5%
Covered Lines: 4249
Relevant Lines: 6922

💛 - Coveralls

@mvomiero mvomiero marked this pull request as ready for review March 6, 2024 17:34
@VinceHeu VinceHeu self-assigned this Mar 7, 2024
@mvomiero
Copy link
Contributor Author

@hohwille thank you for the review and the changes requested 💯

I followed your suggestions for most of the changes, please note just some small details I had to adjust to make the code work:

  • in AbstractEnvironmentVariables I changed a line to fix the bug we had about retrieving variables from IdeVariables
-     if (value != null) {
+     if (value == null) {
        value = var.getDefaultValueAsString(this.context);
      }
  • for the tests, I had to hardcode the version of jasypt in the mocked .jar file name (jasypt-1.9.3.jar instead of jasypt-default.jar). The problem was, that when the commandlet is run, it looks for the artifact of the installed version, and since we input 1.9.3 as version in the properties, it will look for a file with the corresponding name.
    • Since .jar files are included in the gitignore, i had to add a line to exclude jasypt-1.9.3.jar

Some other minor changes:

  • I added a Tag.ENCRYPTION that has a parent Tag.CRYPTO for the commandlet.
  • I changed the name of masterPassword and secret to password and input, which are actually the name of the arguments used when executing jasypt. e.g. java -cp jasypt-1.9.3.jar ... password=xxx input=xxx

Let me know if there are any problems or if something is not clear 😃

@mvomiero mvomiero requested a review from hohwille March 19, 2024 15:43
@hohwille
Copy link
Member

hohwille commented Mar 21, 2024

@hohwille thank you for the review and the changes requested 💯

I followed your suggestions for most of the changes, please note just some small details I had to adjust to make the code work:

* in `AbstractEnvironmentVariables` I changed a line to fix the bug we had about retrieving variables from `IdeVariables`
-     if (value != null) {
+     if (value == null) {
        value = var.getDefaultValueAsString(this.context);
      }

perfect. Thanks for finding and fixing.

* for the **tests**, I had to hardcode the version of jasypt in the mocked .jar file name (`jasypt-1.9.3.jar` instead of `jasypt-default.jar`). The problem was, that when the commandlet is run, it looks for the artifact of the installed version, and since we input 1.9.3 as version in the properties, it will look for a file with the corresponding name.

Correct.

  * Since .jar files are included in the gitignore, i had to add a line to exclude `jasypt-1.9.3.jar`

OK, fine. But in such situations it is better not to do that. Simply use git add -f jasypt-1.9.3.jar instead.

Some other minor changes:

* I added a `Tag.ENCRYPTION` that has a parent `Tag.CRYPTO` for the commandlet.

Fully makes sense.

* I changed the name of `masterPassword `and `secret `to `password `and `input`, which are actually the name of the arguments used when executing jasypt. e.g. `java -cp jasypt-1.9.3.jar ... password=xxx input=xxx`

That is the only issue I have. The parameters from jasypt are very generic and this makes it much more confusing for end-users:
We want to encrypt passwords with a master-password. If a parameter is just called password and both parameters are passwords which of those is it? I personally would prefer here to stay with devonfw-ide compatbility and the from my PoV better understandable parameters masterPassword (the master password for encryption/decryption) and secret (the secret value to encrypt - e.g. a password).

Let me know if there are any problems or if something is not clear 😃

Everything else is perfect - I just have the parameter name concern....

Copy link
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.

@mvomiero thanks for your PR and also for finding and fixing bugs. Great work 👍
I left one comment on the parameter naming. Do you understand my PoV and agree?
If yes, you can simply update and I can merge.
Otherwise lets talk and you can convince me :)

@mvomiero
Copy link
Contributor Author

mvomiero commented Mar 21, 2024

Thank you @hohwille, I totally understand your PoV about the parameters naming. I totally agree with the .gitignore as well, I was not liking the solution either, so I added the file with with git add -f and removed it from the .gitignore
ready to merge then 🚀

@mvomiero mvomiero closed this Mar 21, 2024
@mvomiero mvomiero reopened this Mar 21, 2024
@hohwille hohwille merged commit be4ab47 into devonfw:main Mar 21, 2024
4 checks passed
@hohwille hohwille added this to the release:2024.03.001 milestone Mar 21, 2024
@hohwille hohwille added jasypt Java encryption and decryption tool commandlet labels Mar 21, 2024
@mvomiero mvomiero deleted the feature/26-jasyptCommandletWithUnitTests branch March 22, 2024 12:29
@hohwille hohwille added the story-review marks PRs that will be presented in the sprint-review label May 3, 2024
@tobka777 tobka777 added reviewed Marks PRs that have been presented in the sprint-review meeting or that do not need to be presented. and removed story-review marks PRs that will be presented in the sprint-review labels May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commandlet jasypt Java encryption and decryption tool reviewed Marks PRs that have been presented in the sprint-review meeting or that do not need to be presented.
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Implement ToolCommandlet for Jasypt
5 participants