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

Add consistent explanatory comments #722

Merged
merged 1 commit into from
Jul 8, 2021

Conversation

ghoneycutt
Copy link
Member

No description provided.

@ghoneycutt
Copy link
Member Author

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.

Copy link
Member

@smortex smortex left a 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 from modulesync_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

@ghoneycutt
Copy link
Member Author

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.

@smortex
Copy link
Member

smortex commented Jul 1, 2021

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 😁

moduleroot/.editorconfig.erb Outdated Show resolved Hide resolved
@ghoneycutt
Copy link
Member Author

@ekohl Please merge. VP page is up. Thanks!

@smortex
Copy link
Member

smortex commented Jul 8, 2021

@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?

@ghoneycutt ghoneycutt merged commit e924d92 into voxpupuli:master Jul 8, 2021
@ghoneycutt ghoneycutt deleted the add_explanation_header branch July 8, 2021 19:18
@ghoneycutt
Copy link
Member Author

Thanks everyone for coming together on this! Hopefully this will result in a far fewer issues with new contributors editing these files.

@ekohl
Copy link
Member

ekohl commented Jul 13, 2021

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.

@ghoneycutt
Copy link
Member Author

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.

Comment on lines +1 to +3
# Managed by modulesync - DO NOT EDIT
# https://voxpupuli.org/docs/updating-files-managed-with-modulesync/

Copy link
Member

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.

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.

4 participants