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

Update README-internal #149

Merged
merged 10 commits into from
Jul 31, 2024
Merged

Update README-internal #149

merged 10 commits into from
Jul 31, 2024

Conversation

jazzsequence
Copy link
Contributor

@jazzsequence jazzsequence commented Jul 31, 2024

This pull request updates the internal documentation in the repository.

The changes include:

  • Adding a new section about "Release" and "non-release" commits to clarify the concept and avoid issues with deployment scripts.

  • Updating the language in the differences section to provide clearer explanations.

  • Removing the discouragement of squash commits and specifying that the commit author is rewritten by automation.

  • Updating the documentation for the ci.yml workflow to include information about Playwright tests.

  • Adding a new section about the Playwright tests themselves.

  • Adding a new section about the phpcbf.yml workflow for running the PHP Code Beautifier and Fixer on pull requests.

This PR is blocked by #147 -- the changes the robot fixed were part of what was updated in that PR. Merging this PR before that one will create a commit that gets dropped during deploy.

@jazzsequence jazzsequence requested review from a team as code owners July 31, 2024 16:42
Copy link

Hi from your friendly robot! 🤖 I fixed PHPCS issues with phpcbf on e9befba. Please review the changes.

README-internal.md Outdated Show resolved Hide resolved
## "Release" and "non-release" commits
The composer-managed upstreams (including Drupal (Composer Managed)) have a idiosyncratic concept of "release" or "normal" commits and "non-release" commits. **Release** commits are those that affect files that are ultimately pushed to the `pantheon-upstreams` repository. This includes the everything that makes the base upstream work and _excludes_ any CI-related files, scripts or tests. **Non-release** commits are those that affect files that are not pushed to the `pantheon-upstreams` repository. This includes CI-related files, scripts, tests, and any other files that are not part of the base upstream.

Because we tend to prefer Squash merges on PRs rather than Merge commits, it is vital to separate interests and not combine _release_ and _non-release_ commits in the same PR. Doing so will lead to portions of the PR being dropped when the deploy script is run, leading to a mismatch between the `pantheon-systems` and `pantheon-upstreams` repositories.
Copy link
Member

Choose a reason for hiding this comment

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

Multiple commits in a single PR that keep the division between release and non-release is allowed and encouraged, but should not be squash merged.

This could further be addressed either with a PR template that clarifies a workflow, or a comment out of the existing GH action that comments "Hey I'm going to allow this to happen but don't you dare squash merge"

Copy link
Member

Choose a reason for hiding this comment

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

the comment could get quite noisy on each commit tho , it would have to edit its own comment on subsequent pushes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I will create an issue to extend the Check Commits action to check commits across the PR, too. In the meantime, I can address in the note.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

README-internal.md Outdated Show resolved Hide resolved
@jazzsequence jazzsequence merged commit 0688710 into default Jul 31, 2024
4 of 7 checks passed
@jazzsequence jazzsequence deleted the update-readme-internal branch July 31, 2024 20:26
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.

2 participants