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

Added support for Nested team config #144

Merged
merged 19 commits into from
Jun 19, 2023

Conversation

aadityasinha-dotcom
Copy link
Contributor

Resolves #135
Signed-off-by: aadityasinha-dotcom [email protected]

@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.25 🎉

Comparison is base (ff0a85f) 76.93% compared to head (9ac49e2) 77.18%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #144      +/-   ##
==========================================
+ Coverage   76.93%   77.18%   +0.25%     
==========================================
  Files          32       32              
  Lines        1383     1407      +24     
==========================================
+ Hits         1064     1086      +22     
- Misses        319      321       +2     
Flag Coverage Δ
unittests 77.18% <100.00%> (+0.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/core/zowe/core_for_zowe_sdk/profile_manager.py 83.15% <ø> (-1.22%) ⬇️
src/core/zowe/core_for_zowe_sdk/config_file.py 85.41% <100.00%> (+1.44%) ⬆️
tests/unit/test_zowe_core.py 98.89% <100.00%> (+0.07%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: aadityasinha-dotcom <[email protected]>
@JTonda JTonda requested a review from zFernand0 January 11, 2023 16:03
@t1m0thyj t1m0thyj self-requested a review January 12, 2023 16:10
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

I believe this is going in the right direction.
Can we have some tests added just to verify tht everything works as expected?

src/core/zowe/core_for_zowe_sdk/config_file.py Outdated Show resolved Hide resolved
Signed-off-by: aadityasinha-dotcom <[email protected]>
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

Waiting on tests to be updated to use a nested zowe.config.json 🙂

Signed-off-by: aadityasinha-dotcom <[email protected]>
Signed-off-by: aadityasinha-dotcom <[email protected]>
@JTonda JTonda requested a review from zFernand0 February 22, 2023 16:06
@zFernand0 zFernand0 linked an issue Feb 27, 2023 that may be closed by this pull request
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

Tested this PR with these team config files and the following test script:

from zowe.core_for_zowe_sdk import ProfileManager
profile_manager = ProfileManager(show_warnings=False)

print(profile_manager.load(profile_name="my_zosmf"))
# Expected output: {'host': 'example.com', 'rejectUnauthorized': True, 'port': 443}
# Actual output:   {'host': 'example.com', 'rejectUnauthorized': True, 'port': 443}

print(profile_manager.load(profile_name="lpar1.zosmf"))
# Expected output: {'host': 'example1.com', 'rejectUnauthorized': True, 'port': 443}
# Actual output:   {'host': 'example.com', 'rejectUnauthorized': True}

print(profile_manager.load(profile_name="lpar2.zosmf"))
# Expected output: {'host': 'example2.com', 'rejectUnauthorized': False, 'port': 1443}
# Actual output:   {'host': 'example.com', 'rejectUnauthorized': True}

The first profile "my_zosmf" is loaded correctly, but the two nested profiles "lpar1.zosmf" and "lpar2.zosmf" don't match the expected output.

Please fix the implementation so that the nested profiles are loaded correctly. Happy to chat on Slack if you have any questions 🙂

Signed-off-by: aadityasinha-dotcom <[email protected]>
Signed-off-by: aadityasinha-dotcom <[email protected]>
Signed-off-by: aadityasinha-dotcom <[email protected]>
@zFernand0 zFernand0 requested a review from t1m0thyj March 21, 2023 13:31
t1m0thyj and others added 4 commits March 24, 2023 11:21
Signed-off-by: aadityasinha-dotcom <[email protected]>
Signed-off-by: aadityasinha-dotcom <[email protected]>
Signed-off-by: aadityasinha-dotcom <[email protected]>
Signed-off-by: aadityasinha-dotcom <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

Tested this PR with the sample config and profiles that we've been using. For the my_zosmf profile it is failing to load user and password:
Python SDK: {'host': 'example.com', 'rejectUnauthorized': True, 'port': 443}
Node.js SDK: {"port":443,"host":"example.com","rejectUnauthorized":true,"user":"admin","password":"password"}

The ProfileManager.load method loops through config files in the following order:

  1. Project User Config (./zowe.config.user.json)
  2. Project Config (./zowe.config.json)
  3. Global User Config (~/zowe.config.user.json)
  4. Global Config (~/zowe.config.json)

In the current implementation, profile properties are first loaded from each layer of the config individually, and merged together afterwards.

For the case of our sample config:

  • The user and password properties are stored in the my_base profile of Project User Config (./zowe.config.user.json).
  • This my_base profile is defined as the default base profile in Project Config (./zowe.config.json).
  • At the time when we load Project User Config, we do not yet know that my_base is the default base profile, so the current logic will skip loading its properties. 😢

To fix this, I believe refactoring ProfileManager.load will be necessary. We should first merge all the config layers together, and then load profile properties as a single operation from the resulting merged object.

For a detailed explanation of how config layers should be merged together, see Zowe docs.

@t1m0thyj
Copy link
Member

Proceeding with merging this PR since it supports nested team config in most cases. We can handle the outstanding comment as a separate issue since it requires some significant refactoring of code that's not directly related to this PR: #190

@t1m0thyj t1m0thyj self-requested a review June 19, 2023 19:02
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

Approving per the above comment 😋

@t1m0thyj t1m0thyj merged commit fe48ce3 into zowe:main Jun 19, 2023
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.

Support nested team config files
3 participants