-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add consistent explanatory comments #722
Conversation
Saw voxpupuli/puppet-gluster#228 and realized this is a common occurrence. This PR is meant to add a consistent header to the files to prevent people from editing them within module PR's. |
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.
💯 for the general idea. Some though:
- Should we also include something that says "do not edit this file"?
- While the file is managed by
modulesync
, the actual content come frommodulesync_config
and I think a link to that repository would be more helpful.
This file is managed by [modulesync]. DO NOT EDIT.
Changes to this file must be submitted to the corresponding template in
the [modulesync_config repository], and optionally enabled in a .sync.yml
file at the root of this repository.
[modulesync]: https://github.com/voxpupuli/modulesync/blob/master/README.md#how-it-works
[modulesync_config repository]: https://github.com/voxpupuli/modulesync_config
That's long. Maybe enabling the wiki on the modulesync repo and linking to a page that gives an overview would allow something shorter:
This file is managed by modulesync. CHANGES WILL BE LOST.
https://github.com/voxpupuli/modulesync/wiki/updating-files-managed-by-modulesync
Great ideas @smortex and I'm not attached with what the content of the comment is other than it should be around two lines and not a paragraph at the top. Instead of linking to either repo, perhaps we should have a document on the voxpupuli.org website that explains this process. |
No strong opinion about this: I was thinking that when cloning the repo, the wiki would be cloned too allowing people to have custom instructions on how to edit templates, but this implies modifying the URLs too, so probably does not make much sense :-) I opened voxpupuli/voxpupuli.github.io#245 so that we can iterate on such a page 😁 |
31e1228
to
e728ffd
Compare
@ekohl Please merge. VP page is up. Thanks! |
@ghoneycutt it looks like the commit I added was not gpg-signed and I won't be able to force push to your fork. Can you squash my commit into yours to unblock merging? |
3432a94
to
01cfc20
Compare
Thanks everyone for coming together on this! Hopefully this will result in a far fewer issues with new contributors editing these files. |
My biggest thoughts on this, even though it's been merged. First of all, this makes the difference to https://github.com/voxpupuli/pdk-templates bigger. While merging really isn't realistic in the short term, we should try to remain close. Another thing is that I think we can do a better job. For example, we can create a CI job that runs modulesync on a PR to see if there would be a diff. If there is a diff, it should be reported. #727 would be my preferred step forward. |
Without the headers the experience is CI is that you do a bunch of work and then find out that you should not have edited certain files based on the CI output. @ekohl I like your approach in addition to what we currently have. |
# Managed by modulesync - DO NOT EDIT | ||
# https://voxpupuli.org/docs/updating-files-managed-with-modulesync/ | ||
|
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.
Turns out yardopts does not accept any comments and this breaks things.
No description provided.