Added organizational unit support

[myback]

I added support for organizational groups, similar to user groups. You can specify a group and user that belongs to a specific organizational group.
add the example file examples/addc_ou.json

[phlogistonjohn]

Hi there! Thanks a ton for the contribution. The code looks pretty good on first glance. As you may see now that I've enabled the test run the CI has picked up on the fact that your commit messages are not passing our formatting tests.

In addition I noticed some organizational issues with the commits. In particular the first commit mixes some typo fixes (thanks!) as well as the new feature. I try to have our commits only contain either an improvement of existing code or a new feature when possible.

However, I don't know how comfortable you are with editing existing git commits. If you are good with changing commit messages on your own - the spec is basically topic: description where topic is generally the name of the directory the code changes occur in. If you prefer not to change commit history on your own we can either squash all the changes into one new one, or I can fix up the commit messages (I'd leave the structure of th patches alone in this case). Let me know what you'd prefer. (Edit: another commit linter item that needs resolving: Body does not contain a 'Signed-off-by' line )

Thanks again!

[obnoxxx]

Thanks a lot for this contribution, @myback !

I agree with @phlogistonjohn that it looks pretty good at first glance, apart from the commit message issues that should get fixed before we can approve. In addition to what the CI and @phlogistonjohn already mentioned, I have some additional request:

for the first commit message, please:

• use imperative form instead of past tense, i. e. Add instead of Added.
• include a brief description how to add OUs

[obnoxxx]

@myback wrote:

I added support for organizational groups, similar to user groups.

I think it is important here to distinguish between organizational units (which is what this PR adds support for) and groups which are not OUs. PUs are a bit like subdirectories, but they provide a different dimension of subdividing the AD than groups do.

You can specify a group and user that belongs to a specific organizational group.

also worth mentioning that in addition to specifying users and groups in the definition of an OU, for each user and group definition, an "ou" can now be specified that the object should belong to.

add the example file examples/addc_ou.json

it is good to mention this example file, maybe also in the first/main commit message.

[obnoxxx]

@myback ,

Thanks for splitting the typo fixes out into a separate commit.

I like that approach!

But I'd like to renew my request to use Add instead of Added in thecommit message of the main commit, and please elaborate/explain a bit about the feature in the commit message.

Additionally, both commit messages are still missing Signed-off-by: lines. Please add those.

[myback]

I think it is important here to distinguish between organizational units (which is what this PR adds support for) and groups which are not OUs.

I meant that they are configured in the application in a similar way to groups

done

[obnoxxx]

@myback ,

Thanks for adjusting the commit message and adding sign-off lines!
We're getting closer! 😄

However, I doubt that [email protected] is your real email address. It is an automatic address by github.

Could you please amend the commits adding a proper email?

Finally, an earlier version of the PR had a separate commit that added test(s). What happened to that? Can it be re-added?

[myback]

@obnoxxx ok

[phlogistonjohn]

However, I doubt that [email protected] is your real email address. It is an automatic address by github.

FWIW, I've been generally willing to accept those for people on other projects. It may not be ideal as the point of signed-off-by is saying "I $USER have the legal right to contribute this code". But I have been generally willing to accept them as they can be used to determine the original contributor on github and give the person some additional level of privacy. Just FYI.

[phlogistonjohn]

To save you some time looking at the CI logs: the linter found a formatting issue in tests/test_addc.py

[myback]

To save you some time looking at the CI logs: the linter found a formatting issue in tests/test_addc.py

@phlogistonjohn I only see two empty lines, but these lines were there before https://github.com/samba-in-kubernetes/sambacc/blob/master/tests/test_addc.py#L22-L23

[obnoxxx]

@myback, one more request:

Could you please squash the fix formatting commit with the one that introduces the test_addc.py file? I don't see a good reason for adding the file and fixing the formatting in a later commit in the same PR. If the commit adding the file had already been merged, fixing it in a separate commit in a follow-up PR would be perfectly fine, of course :)

[phlogistonjohn]

Unfortunately it hits a similar error still: tests/test_config.py:312:1: E302 expected 2 blank lines, found 1

In theory, you ought to be able to reproduce the error locally with tox -e formatting. That might save some time rather than having to wait on me to hit the 'Allow CI Run' button on github.

[myback]

An error in formatting the conf-v0.schema.py file occurred due to the fact that I do not have the black tool. There was no error when generating the json schema. But after installing it, the formatting errors did not disappear. Some time later, when I figured out the code, I realized that I could do without generating python code. Is the last commit is good for you?
IMHO it's better to use Pydantic instead of config.py code (i can create another MR for Pydantic)

All done! ✨ 🍰 ✨
61 files would be left unchanged.

[phlogistonjohn]

Thanks for the update. I will re-review soon.

As for pydantic, I was looking at it for a different project recently but decided to skip it because of the more complex dependencies it has and availability on older distros. So while I appreciate the sentiment I think that I'd prefer to avoid it for sambacc for now. Thanks!

[obnoxxx]

Thanks for the updates, @myback !

the CI is happy now and so am I: this PR lgtm now!

[phlogistonjohn]

I'll admit I was bit surprised by the removal of the generated code. It's unfortunate that you had issues with it, but taking it out entirely seems like a big change. On the other hand, I'll admit that having it was possibly a premature optimization. I figured having the schema in python already would avoid some runtime overhead of parsing the json as well as avoiding any packaging issues related to including non-python files[1]. Let me think this over some more before I make a final judgement about this part.

If I decide that I want to keep the generated sources, would you be OK if I tried to help resolve any issues around them? Like by pushing patches into this PR?

[1] - At the very least I'd want to make sure the data files are properly included in the build(s) and we'd probably want to use importlib.resources to load the json file.

[phlogistonjohn]

I have decided that I do not want to drop the generated source file. As part of that decision I made sure that the formatting for the generated python file is not so difficult by adding a tox env and running it. You can see "my version" of you PR here: master...phlogistonjohn:sambacc:wip-pr-104 (the last two patches are new).

If you would like you can either take my changes into your PR, I can push changes into your branch, or I can file an alternate PR that has your original set of change and my additonal patches. Let me know what you'd prefer here. Thanks!

[phlogistonjohn]

See my previous comment. Let's not get rid of the generated python file.

Pull request has been modified.

[myback]

@phlogistonjohn done

[phlogistonjohn]

Sorry for the delay, got distracted and busy. Approved.

[phlogistonjohn]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased