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

Load provider definition dynamically #944

Merged
merged 2 commits into from
Jan 28, 2025
Merged

Load provider definition dynamically #944

merged 2 commits into from
Jan 28, 2025

Conversation

s3rj1k
Copy link
Collaborator

@s3rj1k s3rj1k commented Jan 22, 2025 •
edited
Loading

Adds ability to provide provider configuration from loadable YAML file.

@a13x5
Copy link
Collaborator

a13x5 commented Jan 22, 2025

I would prefer to put provider yamls into ConfigMap instead of baking it into the image.
CM could be added to the kcm chart along with changes to deployment to mount it.

@s3rj1k
Copy link
Collaborator Author

s3rj1k commented Jan 22, 2025

I would prefer to put provider yamls into ConfigMap instead of baking it into the image.

True, but two issues with that:

  • Unit-tests require this to be loaded during testing
  • List has to be loaded during early manager start here, and ensureManagement is only run once if I get this correctly from here

@s3rj1k
Copy link
Collaborator Author

s3rj1k commented Jan 22, 2025

@zerospiel Please also take a look at this

@s3rj1k s3rj1k force-pushed the provider-modules-loadable-list branch from e10c00a to 3d76f9c Compare January 22, 2025 23:17
@s3rj1k
Copy link
Collaborator Author

s3rj1k commented Jan 22, 2025 •
edited
Loading

CM could be added to the kcm chart along with changes to deployment to mount it.

Wait, maybe I misunderstood, just mounting CMs (that have inline YAMLs in them) into image? If so, yea that should work, this was the end-goal actually.

@a13x5
Copy link
Collaborator

a13x5 commented Jan 22, 2025

Wait, maybe I misunderstood, just mounting CMs (that have inline YAMLs in them) into image?

Yes, I meant just move these yamls to CM and mount this CM as a volume in Deployment.
Controller shouldn't get this CM via API it could just read the files in the same way it does now.

@s3rj1k s3rj1k force-pushed the provider-modules-loadable-list branch from 3d76f9c to 9323081 Compare January 23, 2025 11:57
@s3rj1k s3rj1k marked this pull request as ready for review January 23, 2025 13:53
@s3rj1k s3rj1k force-pushed the provider-modules-loadable-list branch from 564a932 to d9c908c Compare January 24, 2025 10:14
eromanova
eromanova reviewed Jan 24, 2025
View reviewed changes
@s3rj1k s3rj1k force-pushed the provider-modules-loadable-list branch from c0acce7 to 8382749 Compare January 26, 2025 11:34
@s3rj1k s3rj1k changed the title Provider modules loadable list via YAML Load provider definition dynamically Jan 27, 2025
@s3rj1k s3rj1k force-pushed the provider-modules-loadable-list branch 2 times, most recently from 535eb23 to 37ea1c4 Compare January 27, 2025 11:19
@s3rj1k
Copy link
Collaborator Author

s3rj1k commented Jan 27, 2025 •
edited
Loading

related: k0rdent/docs#97

@s3rj1k s3rj1k force-pushed the provider-modules-loadable-list branch from 37ea1c4 to ebfd34b Compare January 27, 2025 13:18
eromanova
eromanova reviewed Jan 27, 2025
View reviewed changes
Copy link
Contributor

@eromanova eromanova left a comment

Choose a reason for hiding this comment

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

LGTM. @a13x5 could you please also take a final look and approve the PR if no objections?

@s3rj1k s3rj1k force-pushed the provider-modules-loadable-list branch 2 times, most recently from 74e7a37 to 04b44a3 Compare January 28, 2025 01:25
zerospiel
zerospiel reviewed Jan 28, 2025
View reviewed changes
Copy link
Contributor

@zerospiel zerospiel left a comment

Choose a reason for hiding this comment

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

i have a couple questions, though looks good, I also ask @eromanova and @a13x5 to look on this once again just to ensure everything is also aligned to them

@s3rj1k s3rj1k force-pushed the provider-modules-loadable-list branch from 04b44a3 to 64ab1f7 Compare January 28, 2025 11:03
@s3rj1k s3rj1k force-pushed the provider-modules-loadable-list branch from 64ab1f7 to ecebce9 Compare January 28, 2025 11:10
zerospiel
zerospiel previously approved these changes Jan 28, 2025
View reviewed changes
zerospiel
zerospiel previously approved these changes Jan 28, 2025
View reviewed changes
@s3rj1k
Copy link
Collaborator Author

s3rj1k commented Jan 28, 2025

related: k0rdent/docs#101

eromanova
eromanova reviewed Jan 28, 2025
View reviewed changes
@s3rj1k s3rj1k force-pushed the provider-modules-loadable-list branch from 8f1022d to 2cc93c5 Compare January 28, 2025 15:42
@eromanova eromanova merged commit 5470743 into k0rdent:main Jan 28, 2025
6 checks passed
@s3rj1k s3rj1k deleted the provider-modules-loadable-list branch January 28, 2025 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eromanova eromanova eromanova approved these changes

@zerospiel zerospiel zerospiel left review comments

@Kshatrix Kshatrix Awaiting requested review from Kshatrix Kshatrix is a code owner

@a13x5 a13x5 Awaiting requested review from a13x5 a13x5 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
k0rdent
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@s3rj1k @a13x5 @zerospiel @eromanova