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

Add managed storage support #19

Merged
merged 8 commits into from Aug 22, 2019

Conversation

@piroor
Copy link
Contributor

commented Aug 19, 2019

I'm planning to deploy this add-on to multiple clients in a company with some modified default options. I hope to use the managed storage system instead of modifying of default option definitions. This change will help other people who have same demand. Could you merge this change?

@piroor piroor changed the title Managed storage Add managed storage support Aug 20, 2019

@Peuj

This comment has been minimized.

Copy link
Owner

commented Aug 20, 2019

I've quickly read the documentation (I'm on vacation ^^) and if I understood correctly in managed mode the options cannot be changed by the user.
If it's the case I think I should identify that we are in managed mode and grey the option to "inform" the user that he doesn't have control on the options.
Except that no problem with your request and code.

piroor added 2 commits Aug 21, 2019
Use Array's includes() instead of Set's has().
includes() is slower than has() when there are too many keys, but such an unnatural situation may not happen.
@piroor

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

Thank you for reviewing! I agree that UI elements for managed options should be grayed out, so I've added some more commits. The structure of respond data for the message type getOptions is changed to include locked state of options. How about this approach?

@Peuj

This comment has been minimized.

Copy link
Owner

commented Aug 21, 2019

Perfect! I'll merge and push the update this week end.

@Peuj Peuj merged commit 584cedc into Peuj:master Aug 22, 2019

@piroor

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Thank you for merging!

@Peuj

This comment has been minimized.

Copy link
Owner

commented Aug 23, 2019

No problem :)

I've added some modifications:

Log warning when Managed storage manifest not found because I don't consider this as an error
d477d24

I think it's better to get managedOptions first as localOptions will always be set
deb1547

Let me know if I've missed something.

@piroor

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

Hmm, the second change looks unsuitable for my case. On such case no one can override user's preference. I think that managed storage should be used same as the lockPref() directive in MCD (AutoConfig). lockPref() always apply given value even if there is any user value. On enterprise cases config values defined by the system administrator must be applied for security or other reasons.

@Peuj

This comment has been minimized.

Copy link
Owner

commented Aug 27, 2019

Ok I get it, my bad. I've undo the last change.

If no other problem, I'll push the new version tomorrow.

@piroor

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

Thanks! I have no concern anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.