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.

Sign up for GitHub

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

Jump to bottom

feat: ldap & auxiliary interface integration and some improvements #19

Merged
merged 10 commits into from
Jan 17, 2024
Merged

feat: ldap & auxiliary interface integration and some improvements #19

merged 10 commits into from
Jan 17, 2024

Conversation

wood-push-melon
Copy link
Contributor

@wood-push-melon wood-push-melon commented Jan 5, 2024 •
edited
Loading

This pull request includes the following features:

  • Integration with the ldap interface
  • Improve the configmap propagation delay issue, and use Pydantic for the ldap interface library
  • Improve the development process with pre-commit and mypy
  • Documentations
  • Integration with the glauth-auxiliary interface

Try to keep the charm.py concise and spread the unrelated logic to other modules. However, it seems to me that there is still room to improve charm.py. More effort will be made in the following pull request.

This pull request will wait for the glauth-utils's PR approved and merged.

@wood-push-melon wood-push-melon marked this pull request as draft January 5, 2024 15:26
@wood-push-melon wood-push-melon marked this pull request as ready for review January 8, 2024 15:49
@wood-push-melon wood-push-melon requested a review from a team January 8, 2024 15:49
@wood-push-melon wood-push-melon marked this pull request as draft January 9, 2024 03:40
@wood-push-melon wood-push-melon removed the request for review from a team January 9, 2024 03:40
@wood-push-melon wood-push-melon mentioned this pull request Jan 9, 2024
Merged
@wood-push-melon wood-push-melon changed the title feat: ldap interface integration and some engineering impro feat: ldap & auxiliary interface integration and some improvements Jan 9, 2024
@wood-push-melon wood-push-melon marked this pull request as ready for review January 15, 2024 15:56
@wood-push-melon wood-push-melon requested a review from a team January 15, 2024 15:56
natalian98
natalian98 reviewed Jan 16, 2024
View reviewed changes
Copy link
Contributor

@natalian98 natalian98 left a comment

Choose a reason for hiding this comment

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

Could you add CODEOWNERS file?

src/integrations.py Outdated Show resolved Hide resolved
src/integrations.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
charmcraft.yaml Outdated Show resolved Hide resolved
src/integrations.py Outdated Show resolved Hide resolved
@natalian98 natalian98 requested a review from a team January 16, 2024 10:33
natalian98
natalian98 reviewed Jan 16, 2024
View reviewed changes
config.yaml Outdated Show resolved Hide resolved
@shipperizer
Copy link
Contributor

PR is growing quite big

would we be able to cut it here a6cfcea and basically offload all the StartTLS stuff to another pr? @wood-push-melon

@wood-push-melon
Copy link
Contributor Author

PR is growing quite big

would we be able to cut it here a6cfcea and basically offload all the StartTLS stuff to another pr? @wood-push-melon

Yeah, truncated the certificate-related commit out of this PR, and shift it to another one: #20

@nsklikas @natalian98 I will resolve any certificate-related comments in the PR: #20

natalian98
natalian98 approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@natalian98 natalian98 left a comment

Choose a reason for hiding this comment

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

Would be nice to add more unit tests coverage for the ldap lib, but other than that lgtm

@wood-push-melon wood-push-melon merged commit 20ca07e into main Jan 17, 2024
3 checks passed
@wood-push-melon wood-push-melon deleted the IAM-575 branch January 17, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nsklikas nsklikas nsklikas left review comments

@natalian98 natalian98 natalian98 approved these changes

@shipperizer shipperizer shipperizer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wood-push-melon @shipperizer @nsklikas @natalian98