Initial structure and unit tests for PMR

[kimwnasptd]

Resolves #7

Changes Made

1. Run charmcraft init
2. Strip down the charm code
3. Setup tox and poetry
   1. Use depends = poetry in the testenv of tox, to ensure poetry is always installed
   2. Rely only on poetry groups for dependencies and didn't use depends from tox
   3. Update line-length in black to be 99
4. Create a src/profiles_management module
5. tox unit, format, lint, integration environments
6. Unit tests for PMR class

PMR Class

• Takes as input
  • a dictionary and validate it
  • a file, reads it and validates it
• Constructs a Python object that represents the PMR
• The object provides simple access functions, like pmr.has_profile(name) and profile.has_contributor(name, role)

This class is more defined in the spec.

How to test

sudo add-apt-repository ppa:deadsnakes/ppa
sudo apt update
sudo apt install python3.12 python3.12-venv -y

# create venv and use it
python3.12 -m venv venv
source venv/bin/activate
pip install tox

tox -e unit

[kimwnasptd]

This is blocked on canonical/kubeflow-ci#148. Once that is merged then we can use the action from kubeflow-ci to parse the paths of this repo.

[orfeas-k]

Good job on working towards adopting poetry in CKF charms. Note that I didn't go through the PMR library stuff (classses,schema and their unit tests) since I haven't been involved in the spec. I 'd encourage you to ping people involved in that effort (probably @DnPlas or @mvlassis).

[orfeas-k]

Left some more comments, good job!

[orfeas-k]

LGTM! I 'm not leaving an approval since the PMR class part is yet to be reviewed by someone more involved in that effort.

[DnPlas]

Thanks @kimwnasptd, some comments:

1. We usually keep self contained PRs that are only changing things that are related. I noticed there are a couple of things in this PR that can be grouped and pushed as their own PRs:

• Workflow changes can be split into two: PR 1 for removing the terraform integration tests, PR 2 for using the get paths action.
• All the files that are somehow boilerplate can live in one PR (i.e. .gitignore, CODEOWNERS, tox.ini (to some extent), README (to some extent), CONTRIBUTING (to some extent))
• Adding the charm itself - It looks like you are approaching this repo by creating the PMR code first, then the charm, so I would suggest you avoid all charm code and metadata for now, and also avoid the template as it may introduce false positives in our tests. When you are ready to push the actual charm, then it can live in its own PR.
• PMR code - this can live in its own PR with the code's unit/integration tests, tox.ini changes, workflow changes, requirements, etc., that you need for it to work.

2. Are we using the icon.svg anywhere? I cannot seem to find the place where this is used.

3. It's not very clear to me what is the purpose of the ProfilesManagementRepresentation (PMR) after looking at the code. My understanding was that a PMR is the interpretation of the data that any IdP will provide so the automator charms can convert it into actual Profile CRs. That being said, and in the case of Github, it sounds like users are already providing the PMR and we are just running it through a schema validation to then save it in a dictionary of Profiles, where the keys are the name of the profile, and the values are a Profile object.
   This makes me wonder if we actually need all this machinery for just that or maybe the PMR class will make more sense when we use it with an actual IdP, but then there have to be a couple modifications:

• Considering that we'll not always have a file, potentially we'll get a list (maybe a dict) of users or groups, we'll have to change the PMR class to something that takes that as an argument in the constructor, not just a path.
• Potentially add a couple methods that can return the Profile(s) object so it's ready to be applied with lightkube.

[kimwnasptd]

@DnPlas let's use dedicated comments in the code for multiple points instead of a big one, as now we'll be having huge messages back and forth discussing many points

[kimwnasptd]

1. We usually keep self contained PRs that are only changing things that are related. I noticed there are a couple of things in this PR that can be grouped and pushed as their own PRs:

I've tried to break down the functionality changes to the commit level. I'd suggest we don't go and split the PR as this would take us more time to do the review and all sub-functionalities are very small.

2. Are we using the icon.svg anywhere? I cannot seem to find the place where this is used.

From what I understand, this is what's used in charmhub i.e. https://charmhub.io/kubeflow

3a. It's not very clear to me what is the purpose of the ProfilesManagementRepresentation (PMR) after looking at the code. My understanding was that a PMR is the interpretation of the data that any IdP will provide so the automator charms can convert it into actual Profile CRs

The purpose is to define a common representation, across all IdPs/GitHub, so then we can have a common function create_or_update_profiles(pmr) for handling the PMR. So all Automator charms will have 2 parts:

1. Reading the IdP and creating the PMR dict
2. Initialising a PMR class with the dict: pmr = new PMR(pmr_dict) (validation runs here, charm catches errors and updates status)
3. Running the common create_or_update_profiles(pmr) function

The logic for converting to Profiles, updating the cluster etc is now fully handled by the create_or_update_profiles(pmr) so each Automator charm doesn't have to re-implement this logic, and only focus on reading the IdP and creating the PMR.

3b. That being said, and in the case of Github, it sounds like users are already providing the PMR and we are just running it through a schema validation to then save it in a dictionary of Profiles, where the keys are the name of the profile, and the values are a Profile object

In case of GH yes, the PMR should already be defined in a YAML file. So in this case the charm will

1. create the pmr = new PMR(yaml_file) object, which will also ensure the file is validated
2. run the create_or_update_profiles(pmr) tot update the cluster

3c. This makes me wonder if we actually need all this machinery for just that or maybe the PMR class will make more sense when we use it with an actual IdP, but then there have to be a couple modifications:

For sure once there will be another charm, that will need to convert the data to a PMR will be used more, but nevertheless the library methods (i.e. create_or_update_profiles(pmr)) should be interacting with PMR classes (better dev experience/code: typings, lsp suggestions ect which wouldn't be there if we'd just have dictionaries).

• Considering that we'll not always have a file, potentially we'll get a list (maybe a dict) of users or groups, we'll have to change the PMR class to something that takes that as an argument in the constructor, not just a path.

This could be an option, so the automator charm code can initialise the PMR bottom up, by creating the classes instead of a dict. But I wouldn't rush on extending the PMR's init function yet, until we get to the other charm.

• Potentially add a couple methods that can return the Profile(s) object so it's ready to be applied with lightkube.

Yes, I also had this in mind! But would do this in follow-up PRs and introduce and need these changes as create_or_update_profiles(pmr) function will be developed. So we do it in stages and avoid premature optimisation.

[kimwnasptd]

Adding some notes from today's call with @DnPlas and @mvlassis on this one:

1. We don’t want users to define a dict and then use this to initialise a PMR
2. Users should build the PMR from the ground up, by using the classes directly
3. The PMR class should
   • not have dict, or path inputs. Only a list of Profile class instances
   • Have has_profile, remove_provile and add_profile methods
4. Let's have a README.md for the common code, to expose how it should be used by other charms

(The PMR is essentially a thin wrapper from converting a list of Profile class instances to a dictionary, for efficient checks)

[kimwnasptd]

@DnPlas thanks for the call and the review! I've pushed 2 commits that do the following:

1. Convert all classes, aside from ProfilesManagementRepresentation to be @dataclass
2. Change the __init__ of ProfilesManagementRepresentation to
   1. Not take as input a path or a PMR dict
   2. Take as input a List[Profile] variable
3. Update the ProfilesManagementRepresentation to act as a thin abstraction of having a dict of the Profiles
4. Restructure a bit the schemas
   • I kept all of them, in case in the future some code needs to do a validation of a dict (but am OK with removing them)
   • We only use the ResourceQuota schema, to do a validation of that when the Profile class instance is initialised
5. Restructure the tests, to not do as many validations on PMRs
6. Added a README.md for the common code

bfe2e8e
23b8e35

[DnPlas]

Thanks @kimwnasptd, gave another pass.

[DnPlas]

Re: splitting

Along with @mvlassis and @kimwnasptd, we decided it is okay for now to leave some of the boilerplate along the PMR changes in this PR, but in the future let's make sure to submit self-contained changes.

Re: PMR and the structure

We had an offline discussion and most of the conclusions are in #12 (comment) and #12 (comment)

[orfeas-k]

Regarding splitting the PR, it slipped me while reviewing. I 'm +1 that this is a very large one and normally it would be split into smaller more focused PRs. Given the point where we are though, I 'm OK this time with continuing as is.

[DnPlas]

Thanks @kimwnasptd, good job!

[kimwnasptd]

Force pushed, since the commits were not signed. @DnPlas (or anyone from the team) could you please re-approve?

NIT: I also squashed the review commits from Daniela to one

[mvlassis]

Reapproving after signing the commits, good job @kimwnasptd!