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

Adds CONFIG_USER Filter #1021

Merged
merged 7 commits into from
May 21, 2024

Conversation

abonnell
Copy link
Contributor

Context

Implements a CONFIG_USER Filter that users can hook into and replace key values in their config.yml files

Similar in usage to the CONFIG_UNIQUE filter but it will always replace the value of a given key with the new value being passed in instead of skipping over it

Example usage in a tutor plugin, being used to fetch secrets from AWS Secrets Manager and put that dictionary of key:value pairs into config.yml to be ingested by tutor patches

@hooks.Filters.CONFIG_USER.add()
def fetch_secrets_from_aws(config):
    secrets = s.do_config()

    for secret in secrets:
        config.append(
            (secret, secrets[secret])
        )
        
    return config

Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

thanks for rising up to the challenge! I just have a couple minor comments. Did you try out this change and verify that it works for your use case?

tutor/hooks/catalog.py Outdated Show resolved Hide resolved
tutor/config.py Outdated Show resolved Hide resolved
@abonnell
Copy link
Contributor Author

thanks for rising up to the challenge! I just have a couple minor comments. Did you try out this change and verify that it works for your use case?

Yes! Its working pretty cleanly, no real issues with it within the scope of what its trying to do!

@abonnell abonnell requested a review from regisb March 27, 2024 16:50
@abonnell
Copy link
Contributor Author

@regisb have you had a chance to review after my changes? Thanks! Apologies if I made changes for the review incorrectly, my org doesn't use github and I'm not super familiar with its review process

@DawoudSheraz
Copy link
Contributor

@abonnell Hi. I will add a review on Monday. It has been on my todo, but did not get time to look at it. Apologies for the delay. Thanks

Copy link
Contributor Author

@abonnell abonnell left a comment

Choose a reason for hiding this comment

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

Updated changelog, hook decription, provided clarity to dawoud

@@ -0,0 +1 @@
- [Feature] Introduces the CONFIG_USER Filter. Used to declare unique key:value pairs in config.yml that will be overwritten when running tutor config save. Useful for injecting secrets from an external secrets manager into edx, or other values that may change over time that you need to programmatically fetch. (by @abonnell)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Useful for injecting secrets from an external secrets manager into edx, or other values that may change over time that you need to programmatically fetch --> The filter can be used for injecting secrets from an external secrets manager into Open edX deployment, or other values that may change over time and need to be fetched programmatically

@abonnell
Copy link
Contributor Author

abonnell commented May 9, 2024

Apologies @DawoudSheraz , missed a formatting change. Double checked make test and it seems good now

abonnell

This comment was marked as duplicate.

Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

lgtm!

@DawoudSheraz DawoudSheraz merged commit 2520d93 into overhangio:master May 21, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants