-
Notifications
You must be signed in to change notification settings - Fork 101
feat: Update Hugo and PR template guidance #894
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
base: main
Are you sure you want to change the base?
Conversation
This commit updates the Hugo guidance to accurately reflect the usage of our most common shortcodes, and guides people away from the ones that have been removed or will be removed soon based on Mainframe changes. The guidance is not fully inclusive: we have not yet determined where full documentation for all theme elements should live (Such as a publicly-facing design system), but it should cover the most commonly used shortcodes. The pull request template has also been updated in the same manner, attempting to focus on critical information for contributors. The checklist was previously quite long: it has been reduced to essential elements, and reframed as a direct, personal commitment. The detail underneath the "Proposed changes" has been shifted to inline, hidden guidance, following a similar pattern in our Hugo archetypes. Some information from the old checklist has been moved there, but it otherwise covers the same ground as the old guidance. It refers to and reinforces our standards asserted by our Git conventions documentation, and will not create visual noise within the PR should the contributor choose not to remove it.
✅ Deploy Preview will be available once build job completes!
|
Closes #ISSUE | ||
[//]: # "If the changes were defined by a GitHub issue, reference it using keywords." | ||
[//]: # "https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests" | ||
[//]: # "DO NOT LINK TO ANY INTERNAL, NON-PUBLIC RESOURCES. THIS INCLUDES BOTH INTERNAL REPOSITORY ISSUES OR ANYTHING IN AN INTRANET." |
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'm not sure this is necessary. I think it's understood that even open source companies -- with a public roadmap -- have internal discussions. While at other open source companies, I've marked links as "internal"
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've been trying to minimize my use of internal links. I can say that it's at best "inconvenient". IMO, it adds confusion to discussions with stakeholders.
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 get why this feels unnecessary, but it's also reinforcing the mentality that a commit description should contain all of the context required to understand the set of changes it contains.
Imagine you have checked out the repository, and have no way of learning anything except for what is visible to you in git history. No access to communication channels or any internal documentation.
Giving an internal resource as context for changes is efficient for time but not effective for communication. I solve this for my work by adding or extrapolating relevant details into a public-facing issue or as part of the commit description.
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 therefore suggest:
[//]: # "DO NOT LINK TO ANY INTERNAL, NON-PUBLIC RESOURCES. THIS INCLUDES BOTH INTERNAL REPOSITORY ISSUES OR ANYTHING IN AN INTRANET." | |
[//]: # "Mark any link to internal, non-public resources as INTERNAL" |
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.
Or, based on an INTERNAL discussion:
[//]: # "DO NOT LINK TO ANY INTERNAL, NON-PUBLIC RESOURCES. THIS INCLUDES BOTH INTERNAL REPOSITORY ISSUES OR ANYTHING IN AN INTRANET." | |
[//]: # SECURITY ISSUE: Do NOT add links to internal, non-public resources. This includes both internal repository issues or anything in the F5 intranet. However, it is OK to refer to internal discussions, without links.". |
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.
Giving an internal resource as context for changes is efficient for time but not effective for communication. I solve this for my work by adding or extrapolating relevant details into a public-facing issue or as part of the commit description.
I hear you, but that can lead to two "sources of truth" for discussions. Almost by definition, it's at best difficult to make the same points publicly and privately.
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.
Alternative thought:
- Suggest that internal authors create issues / PRs in the internal repositories, where they can (safely) link to internal discussions
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 hear you, but that can lead to two "sources of truth" for discussions. Almost by definition, it's at best difficult to make the same points publicly and privately.
To an extent, the only real source of truth is what the thing actually does, not what anyone claims or thinks it should, but beyond philosophical pedantry, I treat issues as the source of truth and the git codebase as the enactment of any changes specified by them.
That's part of the definition of done for acceptance criteria and reviews: that the work accurately reflects the intent. If the issue is not a source of truth, then there was not sufficient research or information was captured erroneously.
I think the proposed security-leaning suggestion is needlessly elongating the directive by justifying it: the original change simply says not to link any internal resources. It does not specify you cannot refer to them.
Explicitly listing everything you can or cannot or should or should not do is precisely how the Checklist became long enough in the first place to warrant change: it's noise.
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.
Given the context, I'm continuing discussion internally
(I don't know how to respond without revealing internal information.)
Proposed changes
This commit updates the Hugo guidance to accurately reflect the usage of our most common shortcodes, and guides people away from the ones that have been removed or will be removed soon based on Mainframe changes.
The guidance is not fully inclusive: we have not yet determined where full documentation for all theme elements should live (Such as a publicly-facing design system), but it should cover the most commonly used shortcodes.
The pull request template has also been updated in the same manner, attempting to focus on critical information for contributors. The checklist was previously quite long: it has been reduced to essential elements, and reframed as a direct, personal commitment.
The detail underneath the "Proposed changes" has been shifted to inline, hidden guidance, following a similar pattern in our Hugo archetypes. Some information from the old checklist has been moved there, but it otherwise covers the same ground as the old guidance.
It refers to and reinforces our standards asserted by our Git conventions documentation, and will not create visual noise within the PR should the contributor choose not to remove it.
Checklist
Before merging a pull request, run through this checklist and mark each as complete.
README.md
Footnotes
Potentially sensitive changes include anything involving code, personally identify information (PII), live URLs or significant amounts of new or revised documentation. Please refer to our style guide for guidance about placeholder content. ↩