Skip to content
This repository has been archived by the owner on Mar 17, 2023. It is now read-only.

Improve profile merging for nested dicts #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vpavlin
Copy link
Contributor

@vpavlin vpavlin commented Jun 11, 2019

No description provided.

@vpavlin vpavlin force-pushed the feature/user-spark-configuration branch 8 times, most recently from 372a34c to 3dfe467 Compare June 11, 2019 07:50
@vpavlin vpavlin force-pushed the feature/user-spark-configuration branch from 3dfe467 to 9a4744b Compare June 11, 2019 07:54
@vpavlin vpavlin changed the title Use existing merging for user CM profile Improve profile merging for nested dicts Jun 11, 2019
# lists can be only appended
if isinstance(b, list):
# merge lists
a = list(set(a + b))
Copy link
Contributor

Choose a reason for hiding this comment

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

This will prevent you from implementing list of dicts in the user profiles since default dictionaries aren't hashable

@LaVLaS
Copy link
Contributor

LaVLaS commented Jun 11, 2019

Is the expectation that 2 valid user profiles (userA and userB) can be merged so the merged profile has the combined data from userA and userB? Or is userB's profile the source of truth so that it overwrites anything that exists in userA?

@vpavlin
Copy link
Contributor Author

vpavlin commented Jun 11, 2019

It's complicated:) I would say generally the latter - we have priority list of sources..you could probably mathematically put it as

empty profile < globals profile < any other profile(s) < user specific config map

i.e.start from empty profile, add globals as it has the least priority and sets the base, then layer any other applicable profile and then the user speicific stuff

The problem with lists is that you don't want to duplicate values, which is why I am using sets. As we control the structure of the singleuser profiles I am not sure if we need to care about list of dicts, but it would be a nice to have, but I am not sure how to achieve it:) I a m open to ideas

@LaVLaS
Copy link
Contributor

LaVLaS commented Jun 12, 2019

That makes sense now. I was looking at the changes in isolation and the merge method was restricting the yaml structure without any high level context. It works for the current structure with one edge case where a list/dict could contain stale data since the original destination data isn't being pruned.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants