-
Notifications
You must be signed in to change notification settings - Fork 18
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
refactor(formula): align with template-formula v3.0.0 #12
refactor(formula): align with template-formula v3.0.0 #12
Conversation
Hello. Thanks for your work @noelmcloughlin I must admin than reviewing 54 files change in one commit is quite many to me. I'll take time to do it locally in my GNU Emacs. Regards. |
@noelmcloughlin Could this be done in separate blocks? I.e. One PR specific to |
@baby-gnu, I did not change 54 files - that is misleading claim. The community changed nearly ~50 files in template-formula over past few months so adopting the template formula updates a repo. I changed 7, added 5, deleted 2 approximately. Yes, the PR could have been split. An additional ~5 files have modifications beyond template-formula v3.0.0 state. I'm not familiar with both sysstat and template formulas. Some issues maybe template-formula (i.e. saltstack-formulas/prometheus-formula#3) and some sysstat. The formula has no CI/CD setup so testing is manual and takes a long time. If migrating to template-formula vX.X.X requires reviewing code and patterns already reviewed in template-formula that's is double work - and the effort is doomed. I will try to avoid the bigger PR but its a balance between ideal PR V.S. life, circumstances, and supporting multiple OS/features and expectations. |
@noelmcloughlin I appreciate what you're saying. Reviewing PRs is also hard work and the same situations apply. I've been really busy recently and I can't find the time to review in the way I usually do. So PRs that only do one thing are much easier to deal with. I've collected a list of all of the In the meantime, let's leave this PR as-is and I'll try to get through it over a few days, going through each part. |
I will update the PR description with inventory of where the changes originate from. Filenames originating from template-formula technically do not need a review; During migration I'm raising issues at template-formula and trying to workaround them. I need to raise one issue (saltstack-formulas/prometheus-formula#3) at template-formula because it is a generic issue. just have not got around to it. I apologise for not splitting my commits better ;-) |
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.
@noelmcloughlin This initial review is based on a Travis run and a diff with the template-formula
. I haven't checked the main formula changes themselves.
- https://travis-ci.org/myii/sysstat-formula/jobs/550186931#L1322
jinja2.exceptions.TemplateNotFound: sysstat/osmap.yaml
- Unnecessary since created automatically:
docs/AUTHORS.rst
docs/CHANGELOG.rst
- Add:
bin/kitchen
and add section toREADME
docs/CONTRIBUTING.rst
-- relevant sectionsdocs/TOFS_pattern.rst
- Merge:
.gitignore
.travis.yml
Gemfile
kitchen.yml
You may want to change the commit to be of type feat
since this is more of a minor bump than a patch.
@myii I fixed unit test and tested manually on Ubuntu ( One Debian 9 Travis Job is failing: https://travis-ci.com/saltstack-formulas/sysstat-formula/jobs/210822474 and issue seems to be identical to closed: saltstack/salt#44980 (comment) Anyone ideas? @javierbertoli maybe |
Four travis CI jobs pass. Two have that error. |
@noelmcloughlin
UPDATE: Thanks to @javierbertoli, these are now provided by default in the pre-salted images. The |
@noelmcloughlin As can be seen from the latest run, the final state |
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.
Some changes required, still only general formula structure, more to do with semantic-release
PRs. The actual formula changes can be checked after these changes are made.
Alongside these changes, a couple still from this comment:
- Unnecessary since created automatically:
docs/AUTHORS.rst
docs/CHANGELOG.rst
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.
Thanks for all of the changes, just one that went wrong because my explanation was bad (sorry!). In the meantime, I'll start checking the main formula changes themselves.
@noelmcloughlin Thanks, just need to find some time to check the main formula changes themselves... |
@noelmcloughlin Like saltstack-formulas/deepsea-formula#10, I don't want to hold you up with the merge. I've managed to check the |
🎉 This PR is included in version 0.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This PR is a migration to template-formula v3.0.0. One extra feature added was "optionally install from source tarball". Some unit tests were added.
Verified on Ubuntu with/without pillars. No regression. formula needs further improvements.
sysstat
from source tarball
** clean **