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

Loading of Global and User Configs #113

Closed
wants to merge 16 commits into from

Conversation

shalearkane
Copy link
Contributor

@shalearkane shalearkane commented Aug 19, 2022

Now the Profile Manager class has been split into two parts :

  1. Config File : This just concerns itself with just one file. It handles all the auto-discovering and loading of profiles (prepopulated form base profile properties or just the profile properties). It always raises exceptions when it faces any issues though when it loads secure configs, it suppresses exceptions.
  2. ProfileManager : This class contains the logic to merge the different properties of profiles (from the Project Config and the Project User Config as well as the Global Config and Global User Config). This class handles all the exceptions raised in the Config File to provide a smooth user experience.

Config File is recommended when the user wants to load just one file while ProfileManager automates the merging of profiles.

TODO : Modify tests that checks if correct exceptions are being loaded.

Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
1. ConfigFile will generate exceptions all the time,
it will not try to handle them because we want all the errors to be
handled by the calling function.
2. ProfileManager handles all the exceptions because it is meant to
provide smooth user experience. Hence the load function wraps each
call to the ConfigFile in a try-catch block.

Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
src/core/zowe/core_for_zowe_sdk/profile_manager.py Outdated Show resolved Hide resolved
@@ -0,0 +1,56 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Please add some line and block comments in either this file or the zowe.config.json fixture. The contents of the comments don't matter, they'll just help to verify that ProfileManager is able to load JSON with comments.

src/core/zowe/core_for_zowe_sdk/profile_manager.py Outdated Show resolved Hide resolved
profile_name=profile_name, profile_type=profile_type
)
except Exception as exc:
warnings.warn(f"Could not load Project Config {self.project_config.name}")
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for replacing errors with warnings where the exception isn't fatal and it makes sense to try continuing 👍

For debugging purposes it would be useful to include the exception message in the warning. Could you update this and all the "Could not load X Config" warnings below to include the exception? Something like this:

warnings.warn(f"Could not load Project Config {self.project_config.name}:\n{exc}")

Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
zFernand0 and others added 3 commits September 1, 2022 14:43
Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
exceptions into warnings

fixed tests

Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Base: 64.11% // Head: 66.96% // Increases project coverage by +2.85% 🎉

Coverage data is based on head (098d176) compared to base (904e884).
Patch coverage: 85.41% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             next     #113      +/-   ##
==========================================
+ Coverage   64.11%   66.96%   +2.85%     
==========================================
  Files          28       31       +3     
  Lines        1006     1111     +105     
==========================================
+ Hits          645      744      +99     
- Misses        361      367       +6     
Flag Coverage Δ
unittests 66.96% <85.41%> (+2.85%) ⬆️

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/config_file.py 82.83% <82.83%> (ø)
src/core/zowe/core_for_zowe_sdk/custom_warnings.py 85.00% <85.00%> (ø)
src/core/zowe/core_for_zowe_sdk/profile_manager.py 88.76% <87.14%> (+11.45%) ⬆️
src/core/zowe/core_for_zowe_sdk/__init__.py 100.00% <100.00%> (ø)
...c/core/zowe/core_for_zowe_sdk/profile_constants.py 100.00% <100.00%> (ø)
tests/unit/test_zowe_core.py 98.44% <100.00%> (ø)
src/core/zowe/core_for_zowe_sdk/exceptions.py 75.00% <0.00%> (-7.15%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
@t1m0thyj
Copy link
Member

t1m0thyj commented Sep 6, 2022

Closing in favor of #131

@t1m0thyj t1m0thyj closed this Sep 6, 2022
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.

None yet

3 participants