-
Notifications
You must be signed in to change notification settings - Fork 87
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
Removing profile versioning return value from commands about environment #2053
Conversation
Signed-off-by: Amber Torrise <[email protected]>
Signed-off-by: Amber Torrise <[email protected]>
Signed-off-by: Amber Torrise <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## rm-profile-handlers #2053 +/- ##
=======================================================
- Coverage 90.92% 88.79% -2.13%
=======================================================
Files 616 616
Lines 17165 17153 -12
Branches 3616 3551 -65
=======================================================
- Hits 15607 15231 -376
- Misses 1557 1921 +364
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Smart to have based your branch off of the Changes LGTM! 😋 just gotta address a few failing tests 😅 |
Thanks for going beyond just changing the text that is displayed. The removal of V1 logic in report-env is above and beyond the original issue, but a very worthwhile change. I confirmed that your branch has the same bug fix as the original branch from which you created your branch. So you were right, your test errors are the result of some specific changes in your branch. |
5364277
to
7a1c870
Compare
Signed-off-by: Amber Torrise <[email protected]>
Signed-off-by: Amber Torrise <[email protected]>
…ehind Signed-off-by: Amber Torrise <[email protected]>
Signed-off-by: Amber Torrise <[email protected]>
9ebe5d8
to
0a8b311
Compare
…ank you fernando Signed-off-by: Amber Torrise <[email protected]>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, thanks @ATorrise! Left a few comments about how we handle secure inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 😋
Signed-off-by: Amber Torrise <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the secure values test to handle team config 👍 Left a few comments.
packages/imperative/src/cmd/__tests__/CommandProcessor.unit.test.ts
Outdated
Show resolved
Hide resolved
packages/imperative/src/cmd/__tests__/CommandProcessor.unit.test.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Amber Torrise <[email protected]>
Signed-off-by: Amber Torrise <[email protected]>
Signed-off-by: Amber Torrise <[email protected]>
Signed-off-by: Amber Torrise <[email protected]>
@gejohnston Are these failing tests acceptable or should I do something about them? |
@ATorrise I think that we should merge this branch into rm-profile-handlers. I think that the error will not exist in the rm-profile-handlers branch. Even if there is a problem, it will be easier to fix in that branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if we want to fix the tests in another branch after merge 🙂
Left a comment about why the test may be failing
@@ -2329,6 +2334,7 @@ describe("Command Processor", () => { | |||
envVariablePrefix: ENV_VAR_PREFIX, | |||
definition: SAMPLE_COMMAND_REAL_HANDLER_WITH_OPT, | |||
helpGenerator: FAKE_HELP_GENERATOR, | |||
profileManagerFactory: FAKE_PROFILE_MANAGER_FACTORY, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may need to remove this line now to fix the failing unit tests?
Signed-off-by: Amber Torrise <[email protected]>
What It Does
Part of the modernization effort in preparation for v3. Given v1 profiles will no longer be supported, their consideration in certain command logic is no longer necessary.
How to Test
--show-inputs-only
.zowe config report-env
.Ensure neither command returns anything related to differentiating your environment between
TeamConfig
ofv1
Review Checklist
I certify that I have:
Additional Comments
If there are any other commands the team can think of that return information about environment for user information that have special logic for v1, the removal of that logic could be appended to this PR.