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

feat!: add otp_app option to consumer and producer compile-time options to improve configurating the modules #77

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yordis
Copy link

@yordis yordis commented Dec 20, 2023

No description provided.

Copy link
Contributor

@btkostner btkostner left a comment

Choose a reason for hiding this comment

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

I wonder if we just make otp_app required to deal with config loading. That way apps can't invent their own config loading. Might clean up code.

@yordis
Copy link
Author

yordis commented Dec 26, 2023

I wonder if we just make otp_app required to deal with config loading. That way apps can't invent their own config loading. Might clean up code.

I prefer to use the module scoping rather than relying on global scope such as config :name, ...

@yordis yordis force-pushed the improve-configuring-modules branch from fb900f8 to eb4d3fa Compare December 26, 2023 20:28
@yordis yordis changed the title feat: add otp_app option to consumer and producer compile-time options to improve configurating the modules feat!: add otp_app option to consumer and producer compile-time options to improve configurating the modules Dec 26, 2023
@yordis yordis requested a review from btkostner December 26, 2023 20:28
btkostner
btkostner previously approved these changes Dec 26, 2023
Copy link
Contributor

@btkostner btkostner left a comment

Choose a reason for hiding this comment

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

Code looks good but we will need to set the otp_app for tests

@yordis yordis force-pushed the improve-configuring-modules branch 5 times, most recently from 0c98195 to 0a7445a Compare December 26, 2023 23:47
@yordis yordis force-pushed the improve-configuring-modules branch from 0a7445a to dafb878 Compare December 27, 2023 18:45
@yordis yordis marked this pull request as ready for review December 27, 2023 18:49
@yordis yordis requested a review from a team as a code owner December 27, 2023 18:49
@yordis
Copy link
Author

yordis commented Dec 27, 2023

@btkostner ready!

@doomspork doomspork requested a review from btkostner January 25, 2024 22:39
Copy link
Contributor

@btkostner btkostner left a comment

Choose a reason for hiding this comment

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

Code looks good though this technically is a breaking change and I know there are a couple of services already using it.

Copy link
Contributor

@btkostner btkostner left a comment

Choose a reason for hiding this comment

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

We can make this a non breaking change by making the option not required and defaulting to [] configuration.

@doomspork
Copy link
Contributor

@btkostner that sounds like the way to go 🚀

@terraform-flow terraform-flow bot deleted the branch main February 2, 2024 18:58
@btkostner btkostner changed the base branch from code-freeze/2023-2 to main March 8, 2024 16:11
@btkostner
Copy link
Contributor

Ran into some issues with config loading and required fields. This will require more work to get it in a releasable state without breaking changes.

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.

3 participants