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

PMR: Remove access to stale Profiles with create_or_update_profiles() #17

Merged
merged 6 commits into from
Jan 13, 2025

Conversation

kimwnasptd
Copy link
Contributor

Closes #9

First part of working on the create_or_udpate_profiles function. There will be follow-up PRs for the rest of the functionality of that function, but for now I focused on breaking it to smaller PRs.

Changes

  1. Added helpers for manipulating k8s objects
  2. Started the main create_or_update_profiles(pmr) function
  3. Added integration tests for the create_or_update_profiles(pmr). For now I have one file, but we can break it to multiple as we go

Review Notes

You can test the code by running tox -e integration-library.

There is a bit of duplication between the testing helper functions and the helper functions of the library. I've kept them separate, as it's a bad practice to use utility function from the main logic that is tested from the test scripts.

Lastly, while in the future the function's signature will slightly change, to take into consideration the principals, for now I haven't included them (as they also collide with lint)

@kimwnasptd kimwnasptd requested a review from a team as a code owner December 17, 2024 13:46
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@mvlassis mvlassis left a comment

Choose a reason for hiding this comment

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

Made a first pass, mainly correcting grammar mistakes.

I like the splitting of the files and the small functions, but I'm not sure about the names of the files/directories.

  • Maybe the yamls directory could be renamed to example-yaml to be more descriptive.
  • Not sure about the name of conftest.py
  • The helpers k8s.py, kfam.py, profiles.py could have more descriptive names as well, maybe appending -helpers?
  • I don't think the name create.py fits as well, maybe create_or_update.py is better

@kimwnasptd
Copy link
Contributor Author

Thanks for the review @mvlassis! Regarding the names, indeed naming is always a bit tough.

  • I personally liked yamls as it was concise, what if we do templated_yamls? Since they are not really example yamls
  • conftest is something defined by pytest for sharing fixtures across tests "automagically"
  • Regarding the helpers, I agree, but to avoid adding the -helers I moved all files under the helpers folder/module. WDYT?
  • Don't have a hard preference on create.py to be create_or_update.py

Let's also get feedback from @DnPlas and we can go with the naming that seems more easy to understand for all of us, and I'll make a commit

Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

Thanks @kimwnasptd !

Did a first pass on the create.py file. I think some of my comments may re-shape this PR, so after your consideration and possible update, I can do a second pass on the rest of the files.

src/profiles_management/create.py Outdated Show resolved Hide resolved
src/profiles_management/create.py Outdated Show resolved Hide resolved
src/profiles_management/create.py Outdated Show resolved Hide resolved
src/profiles_management/helpers/k8s.py Outdated Show resolved Hide resolved
src/profiles_management/create.py Outdated Show resolved Hide resolved
@kimwnasptd
Copy link
Contributor Author

I also had to push a commit to address canonical/bundle-kubeflow#1178 and canonical/charmcraft#2023 cc @mvlassis

38ad649

CONTRIBUTING.md Show resolved Hide resolved
@kimwnasptd kimwnasptd changed the title PMR: Remove access to stale Profiles PMR: Remove access to stale Profiles with create_or_update_profiles() Jan 9, 2025
@mvlassis
Copy link
Collaborator

mvlassis commented Jan 9, 2025

@kimwnasptd

  • I think template_yamls seems good, although feel free to use yamls, I don't have any strong feelings on that one
  • The name for conftest.py makes a lot of sense with the context, thanks!
  • I like moving the helpers to their own repository!

Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

Thanks @kimwnasptd, lgtm. Let's also ask @mvlassis for his approval.

@kimwnasptd kimwnasptd force-pushed the kf-6593-remove-access-stale-profiles branch from 7c9d6bb to 49a23c8 Compare January 10, 2025 18:51
@kimwnasptd kimwnasptd merged commit 9f8bdd6 into main Jan 13, 2025
6 checks passed
@kimwnasptd kimwnasptd deleted the kf-6593-remove-access-stale-profiles branch January 13, 2025 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove access from stale Profiles
3 participants