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

#13: implement tool commandlet for aws cli #122

Conversation

MattesMrzik
Copy link
Contributor

@MattesMrzik MattesMrzik commented Oct 27, 2023

resolves #13: The ToolCommandlet for AWS CLI was implemented. Still has a workaround that manually grants executable permissions to the binary under linux. This workaround can be removed if issue #132 is fixed.
This has already PR #140 merged into, since this tool builds upon features from this PR. So probably merge first #140, and then this one here.

Unrelated to the issue #13: Added @TempDir Path tempDir in testSave of EnvironmentVariablesPropertiesFileTest

@MattesMrzik MattesMrzik self-assigned this Oct 27, 2023
@MattesMrzik MattesMrzik changed the title Feature/#13 implement tool commandlet for aws cli #13 implement tool commandlet for aws cli Oct 28, 2023
@MattesMrzik MattesMrzik changed the title #13 implement tool commandlet for aws cli #13: implement tool commandlet for aws cli Oct 28, 2023
@MattesMrzik MattesMrzik marked this pull request as ready for review October 30, 2023 10:03
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.

@MattesMrzik thanks for your PR. Already looks good 👍
I have added some review comments to give some guidance and direction even though your PR is still in WIP/draft state.

cli/src/main/java/com/devonfw/tools/ide/tool/aws/Aws.java Outdated Show resolved Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/aws/Aws.java Outdated Show resolved Hide resolved
@MattesMrzik MattesMrzik marked this pull request as draft November 9, 2023 10:31
@MattesMrzik MattesMrzik marked this pull request as ready for review November 13, 2023 15:40
@MattesMrzik
Copy link
Contributor Author

MattesMrzik commented Nov 13, 2023

This has already PR #140 merged into, since this tool builds upon features from this PR. So probably merge first #140, and then this one here.

Copy link
Contributor

@salimbouch salimbouch left a comment

Choose a reason for hiding this comment

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

Looks good :) I added 2 comments that could make the code a little bit shorter.

@hohwille
Copy link
Member

Should be merged after #139 has been merged.

…plement-ToolCommandlet-for-AWS-CLI

# Conflicts:
#	cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java
…evonfw#13-implement-ToolCommandlet-for-AWS-CLI

# Conflicts:
#	cli/src/main/java/com/devonfw/tools/ide/io/FileAccess.java
#	cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java
#	cli/src/test/java/com/devonfw/tools/ide/io/FileAccessImplTest.java
@coveralls
Copy link
Collaborator

coveralls commented Jan 8, 2024

Pull Request Test Coverage Report for Build 7725548339

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 15 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.2%) to 55.799%

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/commandlet/CommandletManagerImpl.java 3 87.5%
com/devonfw/tools/ide/tool/ToolCommandlet.java 12 45.41%
Totals Coverage Status
Change from base Build 7709887816: -0.2%
Covered Lines: 3650
Relevant Lines: 6301

💛 - Coveralls

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.

@MattesMrzik great job. Looks good. Thanks for testing on Windows and Linux.
Thanks for the helpful comments. Thanks for improving the JUnits. 👍

BTW: You are decreasing the coverage by 0.3%. Pretty much noting but the current strict setup that @moritzLanger has created, prevents merging. So far I like this setup as it enforces us to continuously increase the coverage. However, we could discuss in the team meetings if we want to accept PRs with lets say coverage delta > -0.5%...
To get this merged faster, could you please add a little JUnit test method to increase coverage a very little?

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.

@MattesMrzik sorry - I had a review comment that was pending and I forgot to submit it.
Can you please have a look?

I just rebased the PR and resolved merge conflicts so apart from the last comment we can merge.

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.

ready for merge 👍

@hohwille hohwille merged commit 905710a into devonfw:main Jan 31, 2024
3 checks passed
@hohwille hohwille added the story-review marks PRs that will be presented in the sprint-review label May 3, 2024
@hohwille hohwille added the reviewed Marks PRs that have been presented in the sprint-review meeting or that do not need to be presented. label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commandlet reviewed Marks PRs that have been presented in the sprint-review meeting or that do not need to be presented. story-review marks PRs that will be presented in the sprint-review
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Implement ToolCommandlet for AWS CLI
5 participants