-
Notifications
You must be signed in to change notification settings - Fork 29
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
Release 5.0.0 #222
Release 5.0.0 #222
Conversation
bab5890
to
7cc3ac7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still hoping to get #210 merged before this release.
As it is currently written, it simply works which you can verify by running the unit test, or using the basic acceptance test or any puppet apply and observe the configuration file contents.
This adds value now by warning the user not to make manual edits to settings.py
, and it's particularly valuable to include that change now while we are adding features like tasking system choice and content caching, which are neither simple to configure nor particularly easy to find information about, and could easily break the installation with any manual edits.
In the future I'd like to expand that concept out into a separate puppet module for powerful, flexible, file headers that are reusable across multiple projects but can be overridden in any particular one. We can then use that across our modules to provide targeted hints and documentation links, and it should be generally useful for configuration file headers and particularly when it makes sense to customize or override the header when modules are nested.
I see that as something that could be done later which is not precluded by adding the feature as-is today, in the same way that we have previously written specialized logic directly in these modules to serve a need in the short term, before taking it further upstream e.g. voxpupuli-acceptance.
I understand however that the hesitance could be due to the complexity of some of the logic as well as limited time to review. For that reason, I'll add a few additional tests that should lend some confidence about the robustness of the solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changelog here otherwise looks good.
If it's absolutely necessary to move forward without #210 , I'd like to at least consider whether some less fully-features short-term alternative can be included.
I am going to go ahead and release this. We can always pick up the additional features. |
Potential open PRs:
- [ ] #210