Skip to content

Update CLOSED_CONTRIBUTIONS.md to clarify internal contribution process #449

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
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mjang
Copy link
Contributor

@mjang mjang commented Apr 24, 2025

Proposed changes

Add steps after brainstorming the process with @ADubhlaoich

Closes #444

Checklist

Before merging a pull request, run through this checklist and mark each as complete.

  • I have read the contributing guidelines
  • I have signed the F5 Contributor License Agreement (CLA)
  • 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
  • I have ensured that documentation content adheres to the style guide
  • If the change involves potentially sensitive changes1, 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 and CHANGELOG.md

Footnotes

  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 for guidance about placeholder content.

@mjang mjang requested a review from a team as a code owner April 24, 2025 12:46
@mjang mjang self-assigned this Apr 24, 2025
@github-actions github-actions bot added the process documentation Documentation related to how the repository or documention itself is managed. label Apr 24, 2025
@mjang mjang requested a review from ADubhlaoich April 24, 2025 12:47
Copy link

Deploy Preview will be available once build job completes!

Name Link
😎 Deploy Preview https://frontdoor-test-docs.nginx.com/previews/docs/449/

Once the content changes have been merged in the open repository, they will synchronize back to the closed repository.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone wants to mirror our process, I was tempted to add info about how we've set up jobs/scripts to do the synchronization. IOW, should we say "this doesn't happen automatically"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like a better candidate for documentation around the repository structure and tooling than within this set of discrete instructions.

Someone trying to do closed contributions is trying to get release work done: they don't need the context of the implementation details to fulfill their task.

Copy link
Contributor

@ADubhlaoich ADubhlaoich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Way too much of what I would consider unnecessary information has been added to this set of instructions.

There are some clarifying changes to text I have not commented on since I consider them precise improvements, but the actual "fix" for these instructions is just adding the --all parameter to the fetch command.

Everything else I consider noise: more text does guarantee an improved user experience.

The text itself is written to guide someone towards taking the correct steps each time as a reproducible procedure: expanding the remit of the document to cover a few things that could go wrong invites expansion to documenting everything that could go wrong, and I trust someone interacting with the internal repository has the knowledge to debug any issues caused if they choose to deviate from the procedure.

Comment on lines +41 to +49
```text
Fetching origin
Fetching internal
```

which fetches information from both the:

- `documentation` repository (origin)
- the `internal` repository at the `<closed-url>`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Way too much information. If this document is intended purely for internal contributors, they definitely do not need how git itself works to be re-documented. Adds unnecessary noise to an otherwise concise document.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they definitely do not need how git itself works to be re-documented.

I don't know everything about how Git works, and I've worked at GitLab.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't either, but I know enough to get work done, which is why the document is so tightly scoped.

I put a lot of trust in the idea that the reader has a certain amount of self-sufficiency given they are using git in the first place, and that they mutually put trust in us to not lead them in the wrong direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This process is new to me. I need more information.

To respect your desire for a tighter scope, I've set up progressive disclosure for this section, which you can test in the branch.

Comment on lines 56 to 64

You'll see a prompt with the latest commit hash from the `<closed-url>` repository. You'll something like:

```text
documentation git:(16340b12)
```

It's a good sign. With that hash, you're now working from the `main` branch of the `<closed-url>` repository.
At this prompt, you can now create a feature branch. To comply with NGINX conventions, **Ensure that you prefix all branch names with `internal/`**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More unnecessary information added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this prompt is the big one that scared me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern here is that the amount of information any one of us might find reassuring is noise to anyone else.

I don't even use this process as a result of the work I'm engaged with, but I still think it's important to keep things concise for clarity in respect to the idea that the person most likely to use this process has enough agency and knowledge to figure things out for themselves.

I would like us to start documenting more and more of our processes publicly and there is a cognitive weight to every piece of information added that is not critically important: it sets a precedent of adding more content everywhere for the sake of things that might go wrong.

This is the same reasoning as why we do our best to avoid frequently asked questions: most are assumed instead of actually asked, and they usually come down to a failure of content design or information architecture.

If we got reports from people that they needed reassurance about how git works, I would not push back on this: I never assume that because I find something confusing, everyone else does too, and as a result, try not to assume my lived experience should be what drives the direction of any given piece of documentation.


Change back to the origin `main` branch, create a new branch, merge your internal branch and push to origin.
Change back to the origin `main` branch, create a new branch (`feature`), merge your internal branch, and push to origin.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Branches are not necessarily limited to features: this is an unnecessary caveat.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You did specify internal/feature in the current version of this doc

Suggested change
Change back to the origin `main` branch, create a new branch (`feature`), merge your internal branch, and push to origin.
Change back to the origin `main` branch, create a new branch "such as" (`feature`), merge your internal branch, and push to origin.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used feature as an example name, but I guess that could be updated to internal/example instead.

The existence of a branch means that changes exist, and are not a guarantee the branch has anything to do with features.

Feels like a yak shaving level of nuance.

@ADubhlaoich ADubhlaoich requested a review from a team April 24, 2025 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process documentation Documentation related to how the repository or documention itself is managed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with procedure in CLOSED_CONTRIBUTIONS.md
2 participants