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

Re-organize crates #18

Merged
merged 3 commits into from
May 22, 2021
Merged

Re-organize crates #18

merged 3 commits into from
May 22, 2021

Conversation

katyo
Copy link
Owner

@katyo katyo commented May 8, 2021

No description provided.

cargo readme --no-title --no-indent-headings --no-license --no-badges > README.md
(cd sys && cargo readme --no-title --no-indent-headings --no-license --no-badges > README.md)
- name: Create Pull Request
uses: peter-evans/create-pull-request@v3

Choose a reason for hiding this comment

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

@katyo Maybe this needs if: ${{ github.event_name != 'pull_request' && !startsWith(github.ref, 'refs/tags/') }}? Seems like it would otherwise create a PR with readme update as soon as any CI build runs with documentation changes.

In Smithay/gbm.rs#11 I'm simply failing the build if the readme doesn't match, I prefer a PR to keep the documentation and readme in sync.

Copy link
Owner Author

@katyo katyo May 10, 2021

Choose a reason for hiding this comment

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

@MarijnS95 Of course, this is expected behavior.
Any such automatic created sub PRs should be merged with main PR branch before merging the last (see de17d96).
In such case we don't need editing READMEs manually at all.

Note: Only single sub PR will be created per main PR, on any updates it will be updated too. If no updates required then no sub PRs will be generated.

Copy link

@MarijnS95 MarijnS95 May 10, 2021

Choose a reason for hiding this comment

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

Yes, it can only create a single PR if this pushes to the same branch. But what happens if two PRs change the documentation? Will those override what the CI pushes to the readme PR? IMO it's saner to run it only on master, that way it's only regenerating things that actually changed and have been accepted. Or require contributors to update it, so that changes can be reviewed/cross-validated directly from the PR.

With the CI creating merges across the merge branch (ie. #19 wanting to merge into re-org here) this is only helping users to not have to install+run the command manually. I guess that's... fine? It's not something I'd do for my own projects, that's just way too much noise though.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Updating READMEs via CI is quite useable because contributors may forget to do it manually.
I don't see serious problems in such design excepting some minor disadvantages like this.
In any case PRs needs to be rebased before merging. New update PR will be created when needed.
Even if contributor do not merged sub PR youself it still can be done later after merging main PR on master branch.

Choose a reason for hiding this comment

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

Yeah, that's a fair point. It's all about convenience for contributors vs convenience for maintainers in the end (the alternative being a simple CI failure if the file wasn't updated by the contributor).

Since this PRs to the target branch, does it create a PR in the fork if I were to submit a PR to this repo with a change in the module docs?

Copy link
Owner Author

Choose a reason for hiding this comment

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

The answer is yes because I set a github.head_ref to the base property of create-pull-request@v3 action.

From gh docs:

github.head_ref string The head_ref or source branch of the pull request in a workflow run. This property is only available when the event that triggers a workflow run is a pull_request.

@katyo katyo merged commit 04d3834 into master May 22, 2021
@katyo katyo deleted the re-org branch January 21, 2023 09:10
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