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

[fix] Use Dynaconf lazy variables insted of re-importing the settings. #1130

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rochacbruno
Copy link
Member

@rochacbruno rochacbruno commented Jul 21, 2022

Instead of doing from dynaconf import settings which causes the whole
settings load to be executed again, it is better to use the support for
lazy evaluation and formatting https://www.dynaconf.com/dynamic/#format-token
so it is possible to define a variable that is a template to be formed from
interpolation with other in settings variables.

So inside a settings file that is managed by dynaconf like:

from dynaconf import settings
FOO = settings.BAR + "/zaz"

It is better to do

FOO = "@format {this.BAR}/zaz"

or if Jinja powers is needed

FOO = "@jinja {{ this.BAR }}/zaz"

This way there is no need to reload all the settings and avoids circular dependencies.

[noissue]

@rochacbruno rochacbruno requested a review from a team as a code owner July 21, 2022 15:13
@rochacbruno rochacbruno marked this pull request as draft July 21, 2022 15:30
@rochacbruno
Copy link
Member Author

I will be working on the issue https://github.com/rochacbruno/dynaconf/issues/690 which is a known bug, once that bug is fixed then we can apply this fix to pulp_ansible.

@newswangerd newswangerd removed the request for review from a team October 4, 2022 16:22
@stale
Copy link

stale bot commented Jan 2, 2023

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

@stale stale bot added the stale label Jan 2, 2023
@rochacbruno
Copy link
Member Author

This PR is still missing dynaconf fix first

@stale
Copy link

stale bot commented Jan 2, 2023

This issue is no longer marked for closure.

@stale stale bot removed the stale label Jan 2, 2023
@stale
Copy link

stale bot commented Apr 2, 2023

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

@stale stale bot added the stale label Apr 2, 2023
@rochacbruno
Copy link
Member Author

Dynaconf fix still pending, will be fixed for v4.0 soon

@stale
Copy link

stale bot commented Apr 3, 2023

This issue is no longer marked for closure.

@stale stale bot removed the stale label Apr 3, 2023
@stale
Copy link

stale bot commented Jul 2, 2023

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

@stale stale bot added the stale label Jul 2, 2023
@rochacbruno
Copy link
Member Author

this is probably fixed by dynaconf/dynaconf#944 I will test it

@stale
Copy link

stale bot commented Jul 2, 2023

This issue is no longer marked for closure.

@stale stale bot removed the stale label Jul 2, 2023
@stale
Copy link

stale bot commented Sep 30, 2023

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

@stale stale bot added the stale label Sep 30, 2023
@stale
Copy link

stale bot commented Oct 31, 2023

This pull request has been closed due to inactivity. If you feel this is in error, please reopen the pull request or file a new PR with the relevant details.

@stale stale bot closed this Oct 31, 2023
@rochacbruno rochacbruno reopened this Nov 17, 2023
Copy link

stale bot commented Nov 17, 2023

This pull request is no longer marked for closure.

@rochacbruno
Copy link
Member Author

This PR now depends on Dynaconf 3.3.0 that will include this dynaconf/dynaconf#1040

Instead of doing `from dynaconf import settings` which causes the whole
settings load to be executed again, it is better to use the support for
lazy evaluation and formatting https://www.dynaconf.com/dynamic/#format-token
so it is possible to defina a variable that is a template to be formed from
interpolation with other in settings variables.

So inside a settings file that is managed by dynaconf like:

```
from dynaconf import settings
FOO = settings.BAR + "/zaz"
```

It is better to do

```
FOO = "@Format {this.BAR}/zaz"
```

or if Jinja powers is needed

```
FOO = "@Jinja {{ this.BAR }}/zaz"
```

[noissue]
Copy link

stale bot commented Apr 13, 2024

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

@stale stale bot added the stale label Apr 13, 2024
Copy link

stale bot commented Apr 15, 2024

This pull request is no longer marked for closure.

@stale stale bot removed the stale label Apr 15, 2024
Copy link

stale bot commented Jul 14, 2024

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

@stale stale bot added the stale label Jul 14, 2024
@mdellweg
Copy link
Member

Do we need a versioned dependency on dynaconf here?

Copy link

stale bot commented Jul 15, 2024

This issue is no longer marked for closure.

@stale stale bot removed the stale label Jul 15, 2024
@rochacbruno
Copy link
Member Author

@mdellweg the feature used here is added on dynaconf 3.2.5 https://github.com/dynaconf/dynaconf/releases/tag/3.2.5 which is not available on pulpcore 3.49 https://github.com/pulp/pulpcore/blob/3.49/requirements.txt#L19 (the lower bound for pulp_ansible)

We would need to change main pulp_ansible to require at least pulpcore 3.50.

@mdellweg
Copy link
Member

@mdellweg the feature used here is added on dynaconf 3.2.5 https://github.com/dynaconf/dynaconf/releases/tag/3.2.5 which is not available on pulpcore 3.49 https://github.com/pulp/pulpcore/blob/3.49/requirements.txt#L19 (the lower bound for pulp_ansible)

We would need to change main pulp_ansible to require at least pulpcore 3.50.

I thought, we could require that dynaconf version here until we bump pulpcore to >= 3.50.
But we can also wait. What would be more convenient?

@rochacbruno
Copy link
Member Author

I think we cannot require the version here because pulpcore 3.49 has upper bounds for dynaconf.

Maybe we can just wait the proper time to merget this, this is not blocking anything, it is just adding a bit of overhead on application startup time.

Copy link

stale bot commented Oct 17, 2024

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

@stale stale bot added the stale label Oct 17, 2024
@rochacbruno
Copy link
Member Author

I think I am ready to fix it now.

Copy link

stale bot commented Oct 17, 2024

This issue is no longer marked for closure.

@stale stale bot removed the stale label Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants