#1135: Add PowerShell environment initialization - #2232
Conversation
2d5eee9 to
d7bac43
Compare
Coverage Report for CI Build 31709859615Coverage increased (+0.06%) to 72.946%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions86 previously-covered lines in 1 file lost coverage.
Coverage Stats💛 - Coveralls |
There was a problem hiding this comment.
Good job! IMHO this is a nice implementation which adds the functionality requested in #1135. I've ran through your test instructions and everything worked perfectly fine. I have a small suggestion though concerning your method structure. Also you could think about adding tests for the changes you made.
e019779 to
137bc42
Compare
samuelkos17
left a comment
There was a problem hiding this comment.
Thanks for applying my suggestions and writing Unit Tests! However I still think you should change some very minor things. I also suggest that you write an integration test to actually make sure that the file gets modified on disk.
48ee6c2 to
d8642f1
Compare
samuelkos17
left a comment
There was a problem hiding this comment.
Thanks for changing again, this seems good to me now so I'll move it to In Review. 😄
d8642f1 to
b49d20d
Compare
b49d20d to
3d8164c
Compare
3d8164c to
a21b15a
Compare
hohwille
left a comment
There was a problem hiding this comment.
@cap-juan thank you for your PR. This is really great work. You implemented an analogue variant of functions for powershell as functions.ps1 and integrate this with PowerShell during the installation of IDEasy. 👍
Of course the cost of all this is that we now have duplicated maintenance effort for all changes made to functions also in functions.ps1 - esp. since you implemented all features including icd (but AFAIK excluding auto-completion).
However, I would say that we meanwhile reached a state where things like that are somewhat stabilised. Still I fear that with stories like #2050 we might later get some pain with this.
Therefore my question is what will be our new status for PowerShell that was currently "unsupported":
IDEasy/cli/src/main/package/bin/ide.bat
Lines 72 to 73 in fc9df60
Do we now also want to build full auto-completion support, etc.?
My initial idea was to rather educate users to use bash as a universal shell:
#1430
| modifyPowerShellProfile("powershell", install); | ||
| modifyPowerShellProfile("pwsh", install); |
There was a problem hiding this comment.
This code will also run in JUnits and however tweak the real end-users powershell setup.
We want to avoid such side-effects and ensure IdeTestContext ensures encapsulation.
I would therefore suggest to extend this special feature to WindowsHelper that has its own Mock implementation so we do not manipulate or even "destroy" the end-users environment when he executes JUnit tests.
Or am I missing something and this is already mocked away via ProcessContext so the exeuctions of powershell and pwsh will more or less be void?
| * https://github.com/devonfw/IDEasy/issues/1135[#1135]: IDEasy does not set env variables on Windows PowerShell | ||
| * https://github.com/devonfw/IDEasy/issues/1135[#1135]: Fix PowerShell env variable initialization on Windows by sourcing functions from the PowerShell profile |
There was a problem hiding this comment.
Why was the issue added twice with different titles?
Also this needs to be moved up to the most current release (Sorry, I was too late with reviews).
This PR fixes #1135
Implemented changes:
Testing instructions
Check out the PR branch and build/install IDEasy using the normal local development workflow.
Verify that the PowerShell integration script was installed:
Test-Path "$env:IDE_ROOT\_ide\installation\functions.ps1"The command should return True.
Close all existing PowerShell sessions.
Open a new PowerShell window directly inside an IDEasy project.
Verify that ide is loaded as a PowerShell function:
Get-Command ideThe command type should be Function.
Verify that the IDEasy environment was initialized automatically:
Both variables should contain the values for the current IDEasy project.
Verify that Maven can be executed without running ide first:
mvn --versionVerify manual environment refresh after changing directories:
Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internalChecklist for tool commandlets
Have you added a new
«tool»as commandlet? There are the following additional checks:«tool»«TOOL»_VERSIONand«TOOL»_EDITIONare honored by your commandlet