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
PR - Issue 50041 - CLI and WebUI - Add memberOf plugin functionality #3234
Comments
Comment from spichugi (@droideck) at 2019-01-18 19:32:58 The current state of the PR is intermediate. I'll add 'fixup' task and 'config entry - manage' part i a couple of following days. |
Comment from spichugi (@droideck) at 2019-01-18 19:34:33 Also, you need to reinstall the node_modules for 'react-bootstrap-typeahead' package. |
Comment from spichugi (@droideck) at 2019-01-18 21:34:54 rebased onto 5e6c009575cbac9bd1bbf5960645443497249bd2 |
Comment from firstyear (@Firstyear) at 2019-01-20 22:56:59 This shouldn't be choices, because it could be any oc that takes MO. |
Comment from firstyear (@Firstyear) at 2019-01-20 22:58:33 What's an example of this command in use? dsconf localhost plugin memberof edit --attr ? |
Comment from firstyear (@Firstyear) at 2019-01-20 23:08:06 Okay, I'm not sure about generic edit. I'm currently working on "modify" in another ticket, that will perform the same. Part of the design, of having the ability to "wrap and set" attributes (which you have removed in this patch), is that it's really clear what can and can't be altered in a really natural way. Another aspect is the cli should do "lots of common tasks" but "not everything". We should be offering only common useful settings, rather than everything. The cli offers a safe modification interface. A benefit of your change is "batch" changes, where you change many attributes at once. However, I would prefer that there is a "delete" keyword rather than "no string" meaning delete. I think that deletes should always be a requested action, not an implied actionn. The design of this --edit will mean any mistake on the persons part where they forgot to write an argument will cause harm. Any situation where a person could "hold it wrong" means we have failed as tool designers. So I would say these are the options I request: leave the CLI as is and use the "modify" interface that I'm adding, which does what you want for "advanced" users, and has a better, clearer delete syntax. Or, implement this change, but have a clear "delete" keyword when you want to remove an attribute, rather than the current method of empty string."--attr " // "--attr delete". Enforce that each --attr takes 1 or more arguments (if more is valid). |
Comment from spichugi (@droideck) at 2019-01-21 11:14:08
Okay, it had these choices before - so I left it as is. But I think you are right.
Yes
I think the way I have it (and other CLI tools that Mark was implementing too - we use the same way everywhere) is also able to limit a user. Like, you can edit only the attributes that were explicitly specified in the '--' flags. If the administrator would like to set up something, he will open the help and he will see what can be set.
Okay, I was thinking about that too. I will change it. So in general, the way with batch changes is the way we have it now for some time. Mark and I was doing it for other parts of the CLI and I think we should keep at least because of the two reasons:
|
Comment from mreynolds (@mreynolds389) at 2019-01-21 20:48:46 Using "print" was on purpose. I think you can not redirect the output using log.info(). There was some problem with it that was only fixed by using print for CLI output. |
Comment from firstyear (@Firstyear) at 2019-01-21 22:56:12
Just don't try and enforce anything, just allow any objectClass here, but recommend that nsMemberOf is probably what they want in most cases.
Yeah. Batching makes sense here. Check my patch in #3217 because I think you can use this instead of generic edit here. Perhaps what is needed is a way to display from the "objectclass" the valid attributes from the schema (Which is why it's important we stop using extensible object ...). Saying this, the two could be complimentary, rather than exclusive as well. We could keep edit with the list of valid options, and detailed help, and have modify as a raw interface to the objects? That could also get confusing in some cases too .... PS: My patch in 50158 fixes some behaviour in mod_delete actions, so you probably will need to rebase and test this. :) |
Comment from spichugi (@droideck) at 2019-01-22 03:00:40 rebased onto 129f9b6eab3781aacb17cf650c69113499e9fa6e |
Comment from spichugi (@droideck) at 2019-01-22 03:04:41
Okay, I'll check it next thing. Thanks! |
Comment from firstyear (@Firstyear) at 2019-01-22 22:49:06 Great, thanks! Let me know what you think of that PR as well. I can't say much about the JS here so I'll leave @mreynolds389 for that part :) |
Comment from firstyear (@Firstyear) at 2019-01-22 22:59:19 PS: We still should think about modify vs edit interface for these calls by the way. I think that perhaps on reflection, leave the current code (so roll back your py change), then have the js use the modify interface in 50158? Would that be more complex? Or better? It allows you to perform arbitrary modifications you want .... |
Comment from spichugi (@droideck) at 2019-01-23 10:32:31
I think it is better with options (as we have consistently over most CLI areas that Mark and me have added). It gives nice discoverability with |
Comment from spichugi (@droideck) at 2019-01-23 11:57:44 2 new commits added
|
Comment from spichugi (@droideck) at 2019-01-23 11:59:33
Right... I didn't use the redirection with these tools but someone could need to. Please, review! |
Comment from firstyear (@Firstyear) at 2019-01-23 12:20:20
This is a reasonable concern. I think it's hard because I have a different approach, but to make everything consistent would be breaking. So how about we do the "edit" as you have here, and the "modify" from my other patch? Otherwise, with this in mind, I'm happy with the python. Again, please rebase to my change in 50158 due to delete semantics, I'll let @mreynolds389 review the js. Thank you for being patient with me :) |
Comment from mreynolds (@mreynolds389) at 2019-01-25 16:23:19 Hi Simon, so there are some issues around "saving" the configuration because you are not maintaining a before and after snapshot of the settings. So if I do nothing and click save it actually rewrites the plugin entry in DS. What I do here is I maintain two sets of the state attributes. Current and original, then on save I check if current is different than the original. If they are different I update the cmd list and perform the update. Something like this:
So when I "load" the config I set both state attributes, then "handleChange" updates the non "_" variable. Finally we compare them at "save" time, and only update what changed (if anything changed at all) Also fixup task modal does not reset when it is reopened. If I put a value in the modal and cancel, then reopen it, the value is still present in the input field. Also the "Config Entry" field, should be "Shared Config Entry". The rest looks great! |
Comment from mreynolds (@mreynolds389) at 2019-01-25 16:25:34 Also, after making a config change (success or failure) I reload the config (which re-renders everything with the correct values and resets previous values ( --> the "_" state variables). |
Comment from mreynolds (@mreynolds389) at 2019-01-25 16:28:59 Also, by reloading the config after a successful or failed update means you don't have to maintain the html after a update because its always reset(re-rendered) from scratch. Well that's how I'm doing it in the database tab, and it works nicely, and actually creates less work. |
Comment from spichugi (@droideck) at 2019-01-25 17:47:13 Nice catch! Yeah, actually I've noticed the issue before I switched to the docs and presentations work. |
Comment from spichugi (@droideck) at 2019-01-28 15:54:25 rebased onto eaf4bae4cbdc0c5ee184dd415ee931b52de81d8a |
Comment from spichugi (@droideck) at 2019-01-28 15:57:53 Okay, I fixed it a bit differently. The rest is fixed too. |
Comment from mreynolds (@mreynolds389) at 2019-01-28 16:37:28 Well I disagree with your stance on using state to know if a value changed. Typically configuration is very static, and is only changed by one person. The odds of the memberOf plugin being changed between you opening the plugin (which should get fresh values), and clicking save is very slim. Now, you are performing many operations even if the config did not change. My other concern here is attributes without values (or missing attributes), you will try to save empty-values which will fail for some attribute syntax checks (DirectoryString verses IA5String). In this case it does it correctly, but could this approach could fail for other configurations? That being said, both approaches have pro's and con's - just proceed with caution if you continue to use this approach for other areas of the server. Ack! |
Comment from spichugi (@droideck) at 2019-01-28 18:34:02 rebased onto 614ab2a |
Comment from spichugi (@droideck) at 2019-01-28 18:34:09
I agree. I just wanted to have more precautions... Anyway, in the future, I'd like to have the UI to be in sync with the instance.
I also check that we don't save any empty values. If the user set the field to an empty value, I assume the user would like to remove/not have the attribute in the entry.
Thanks! |
Comment from spichugi (@droideck) at 2019-01-28 19:07:09 Pull-Request has been merged by droideck |
Comment from mreynolds (@mreynolds389) at 2019-01-28 19:46:02
What happens when it tries to delete an attribute that is not present in the entry? Is the error ignored, or is the operation rejected? |
Comment from spichugi (@droideck) at 2019-01-28 20:41:45
CLI will filter the attribute from the modlist: |
Comment from mreynolds (@mreynolds389) at 2019-01-28 20:55:14
Great! |
Patch |
Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50175
Description: Add mainfunctionality to memberOf plugin tab.
Increase the eslint max line length from 80 to 100.
Rework plugin properties to be more compact.
Eslint webpack config. Add react-bootstrap-typeahead for
multivalued attributes. Fix the word 'successfully' typos.
Resolves: #3100
Reviewed by: ?
The text was updated successfully, but these errors were encountered: