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

Fixes profile merge order to match Node.js SDK #203

Merged
merged 16 commits into from
Oct 2, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ All notable changes to the Zowe Client Python SDK will be documented in this fil

## Recent Changes

- Feature: Added method to load profile properties from environment variables
- Bug: Fixed profile merge order to match Node.js SDK
- Feature: Added method to load profile properties from environment variables
1 change: 1 addition & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ chardet==4.0.0
colorama==0.4.4
commentjson==0.9.0
coverage==5.4
deepmerge==1.1.0
flake8==3.8.4
idna==2.10
importlib-metadata==3.6.0
Expand Down
8 changes: 8 additions & 0 deletions src/core/zowe/core_for_zowe_sdk/profile_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import os
import warnings
from typing import Optional
from deepmerge import always_merger

from .config_file import ConfigFile, Profile
from .custom_warnings import (
Expand Down Expand Up @@ -259,10 +260,17 @@ def load(

missing_secure_props = [] # track which secure props were not loaded

loaded_cfg: dict = {}

for i, (config_type, cfg) in enumerate(config_layers.items()):
loaded_cfg = always_merger.merge(loaded_cfg, cfg.profiles)
Copy link
Contributor Author

@aadityasinha-dotcom aadityasinha-dotcom Aug 9, 2023

Choose a reason for hiding this comment

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

I did use the deepmerge library so that it can merge each layer as a whole config

Copy link
Member

@t1m0thyj t1m0thyj Aug 10, 2023

Choose a reason for hiding this comment

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

Not all config layers can be merged with the same merger function. See the Node.js SDK for how profiles of each layer should be merged:

  1. Project and Project User are deep merged into a temporary Project Merged layer
  2. Global and Global User are deep merged into a temporary Global Merged layer
  3. Add profiles to Project Merged layer which are only present in Global Merged layer

if loaded_cfg:
cfg.profiles = loaded_cfg

profile_loaded = self.get_profile(
aadityasinha-dotcom marked this conversation as resolved.
Show resolved Hide resolved
cfg, profile_name, profile_type, config_type
)
# How about we update by iterating each layer and at last we will get the merged layer
# TODO Why don't user and password show up here for Project User Config?
# Probably need to update load_profile_properties method in config_file.py
if profile_loaded.name and not profile_name:
Expand Down