generated from nginx/template-repository
-
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
Open
ADubhlaoich
wants to merge
2
commits into
main
Choose a base branch
from
docs/hugo-pr-guidance
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,35 +1,28 @@ | ||
### Proposed changes | ||
|
||
Write a clear and concise description that helps reviewers understand the purpose and impact of your changes. Use the | ||
following format: | ||
[//]: # "Write a clear and concise description of what the pull request changes." | ||
[//]: # "You can use our Commit messages guidance for this." | ||
[//]: # "https://github.com/nginx/documentation/blob/main/documentation/git-conventions.md#commit-messages" | ||
|
||
Problem: Give a brief overview of the problem or feature being addressed. | ||
[//]: # "First, explain what was changed, and why. This should be most of the detail." | ||
[//]: # "Then how the changes were made, such as referring to existing styles and conventions." | ||
[//]: # "Finish by noting anything beyond the scope of the PR changes that may be affected." | ||
|
||
Solution: Explain the approach you took to implement the solution, highlighting any significant design decisions or | ||
considerations. | ||
[//]: # "Include information on testing if relevant and non-obvious from the deployment preview." | ||
[//]: # "For expediency, you can use screenshots to show small before and after examples." | ||
|
||
Testing: Describe any testing that you did. | ||
|
||
Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide | ||
specific feedback, add them here. | ||
|
||
If this PR addresses an [issue](https://github.com/nginx/documentation/issues) on GitHub, ensure that you link to it here: | ||
|
||
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." | ||
|
||
### Checklist | ||
|
||
Before merging a pull request, run through this checklist and mark each as complete. | ||
Before sharing this pull request, I completed the following checklist: | ||
|
||
- [ ] I have read the [contributing guidelines](https://github.com/nginx/documentation/blob/main/CONTRIBUTING.md) | ||
- [ ] I have signed the [F5 Contributor License Agreement (CLA)](https://github.com/f5/.github/blob/main/CLA/cla-markdown.md) | ||
- [ ] I have rebased my branch onto main | ||
- [ ] I have ensured my PR is targeting the main branch and pulling from my branch from my own fork | ||
- [ ] I have ensured that the commit messages adhere to [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/#summary) | ||
- [ ] I have ensured that documentation content adheres to [the style guide](/documentation/style-guide.md) | ||
- [ ] If the change involves potentially sensitive changes[^1], I have assessed the possible impact | ||
- [ ] If applicable, I have added tests that prove my fix is effective or that my feature works | ||
- [ ] I have ensured that existing tests pass after adding my changes | ||
- [ ] If applicable, I have updated [`README.md`](/README.md) | ||
- [ ] I read the [Contributing guidelines](/CONTRIBUTING.md) | ||
- [ ] My branch adheres to the [Git conventions](/documentation/git-conventions.md) | ||
- [ ] My content changes adhere to the [F5 NGINX Documentation style guide](/documentation/style-guide.md) | ||
- [ ] If my changes involve potentially sensitive information[^1], I have assessed the possible impact | ||
- [ ] I have waited to ensure my changes pass tests, and addressed any discovered issues | ||
|
||
[^1]: 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](/documentation/style-guide.md) for guidance about placeholder content. | ||
[^1]: Potentially sensitive information includes personally identify information (PII), authentication credentials, and live URLs. Refer to the [style guide](/documentation/style-guide.md) for guidance about placeholder content. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
Uh oh!
There was an error while loading. Please reload this page.
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:
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.
Uh oh!
There was an error while loading. Please reload this page.
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:
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.)