-
-
Notifications
You must be signed in to change notification settings - Fork 62
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 script to add PR & Issue templates to all IaC repos #58
Conversation
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.
Overall, LGTM. Just left a bunch of NITs.
} | ||
|
||
function create_pr_template { | ||
cat << "EOF" > .github/pull_request_template.md |
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.
NIT: should these be .md
files checked into this git-xargs
repo? You could then use cp
to copy them to the proper destination. It would then make it easier to read / edit / preview those files than when they are inlined within a Bash script.
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.
@brikis98 we can, but it would be the first script example that copies external files to a repo. I guess this is not such a bad thing, but it will also make the script slightly less portable. I.e. The user would need to ensure they copy a folder containing the script and all of the required templates for it to work 100% correctly. We could fail hard if one of the templates is missing. @zackproser what are your thoughts?
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.
Moving the markdown files to the git-xargs
repo would make them slightly easier to read, but we'd also have the extra step of ensuring we've updated and pulled on the repo each time for running the latest script, as Rob points out. I don't have a strong opinion here, so I'd say go with whatever is easier for you.
Makes me think git-xargs
might do well with a means of fingerprinting the script or command it ran each time and adding that info to a PR description or something - for easier coordination in the common workflow of needing to make several small updates to a script following the first couple of runs. This would make it easier for colleagues to know which version of a script generated which PRs.
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.
Makes me think git-xargs might do well with a means of fingerprinting the script or command it ran each time and adding that info to a PR description or something - for easier coordination in the common workflow of needing to make several small updates to a script following the first couple of runs. This would make it easier for colleagues to know which version of a script generated which PRs.
Yeah, that's a good idea. We've been in a very similar territory recently planning an internal project. It appears RenovateBot actually stores a fair bit of state using pull requests. i.e: It knows whether a repo has been onboarded or not based on the status of a PR. I think it would be handy if git-xargs
could by default sprinkle some metadata into the PRs.
Question: are the data batches containing Gruntwork repos of general interest to git-xargs users, or should we migrate them to our internal prototypes repo, and leave the script itself here? Doing so would increase our own overhead for running the script slightly, but perhaps it's worth considering if the open-source community is unlikely to find the Gruntwork repo batches useful. Not a showstopper IMO, but thought I'd bring it up. |
Yes inline with our Slack conversation, I'll move the data batches over to the |
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.
LGTM 👍
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.
LGTM
For the record I'm going to accept this PR as is and move on. |
This is part of our new coding methodology rollout plan.