Skip to content

Conversation

dscho
Copy link
Member

@dscho dscho commented Sep 1, 2025

GitGitGadget's GitHub Actions all implicitly use the defaultConfig that hard-codes the configuration of the OG GitGitGadget that supports the Git project (and the Git project only).

However, for a long time there have been feature requests and musings towards using GitGitGadget also for other projects.

GitGitGadget already does have some beginnings of a support for other projects, e.g. the IConfig interface. With these changes, that interface gets extended a little, a validator is added that can verify whether or not an arbitrary object conforms to that interface, and then new inputs are introduced, accepting this (JSON-encoded) configuration as a user-provided string. The idea is that a project-specific fork of https://github.com/gitgitgadget/gitgitgadget-workflows/ contains this configuration in the gitgitgadget-config.json file in a dedicated config branch, from where it is synchronized via a GitHub workflow to the repository variable called CONFIG.

This somewhat non-trivial setup allows the config to be conveniently tracked via Git, updated via Pull Requests, validated via GitHub workflows, and still to be used in a trivial manner in the main workflows via ${{ vars.CONFIG }} (as opposed to having to play games with multi-branch checkouts or curl'ing a file from a different branch).

This PR is step number 2 in that direction. Step number 1 was to register the cygwingitgadget GitHub org for experimenting with GitGitGadget support for a different mailing-list-based project than Git: Cygwin.

That org is where I experimented with this change as well as with all the others leading up to gitgitgadget/gitgitgadget-github-app#7.

This PR is stacked on top of #1993 and #1994

@dscho dscho requested a review from webstech September 1, 2025 16:58
@dscho dscho self-assigned this Sep 1, 2025
export interface IRepoConfig {
name: string; // name of the repo
owner: string; // owner of repo holding the notes (tracking data)
baseOwner: string; // owner of upstream ("base") repo
Copy link
Member Author

Choose a reason for hiding this comment

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

@webstech since I started referring to the corresponding repository as upstream-repo, what would you think about the idea to rename this from baseOwner to upstreamOwner?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no issue with renaming. My choice in names may not always be as git oriented as they could be.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the record, I am going forward with this: dscho@2de5309

owner: string; // owner of repo holding the notes (tracking data)
baseOwner: string; // owner of upstream ("base") repo
testOwner?: string; // owner of the test repo (if any)
owners: string[]; // owners of clones being monitored (PR checking)
Copy link
Member Author

Choose a reason for hiding this comment

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

@webstech what do you think about the idea to move this attribute to IAppConfig and to rename it to installedOn? That way, the repo still can have an upstreamOwner on whose repository, however, GitGitGadget is not installed...

Copy link
Contributor

Choose a reason for hiding this comment

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

This is used several times in CIHelper so the location could be justified. On the other hand, move it if you like.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the in-development patch to do so: dscho@9c2e794.

@dscho
Copy link
Member Author

dscho commented Sep 2, 2025

Note to self: check whether default: ${{ vars.CONFIG }} works, and would avoid the need for the explicit config: ${{ vars.CONFIG }} lines added in gitgitgadget/gitgitgadget-workflows@2952485.

@webstech
Copy link
Contributor

webstech commented Sep 4, 2025

An aside while I was looking at this, I noticed CIHelper.parsePRURLInput() could be using getPullRequestKeyFromURL.

dscho added a commit to cygwingitgadget/gitgitgadget-workflows that referenced this pull request Sep 8, 2025
In gitgitgadget/gitgitgadget#1991, I adjusted
and augmented the `IConfig` interface a bit. Concretely, I renamed
`repo.baseOwner` to `repo.upstreamOwner` in 2de5309e (IConfig: rename
the attribute defining the `upstream-repo`'s org, 2025-09-08) and moved
`repo.owners` to `app.installedOn` in 9c2e7944 (IConfig: move
`repo.owners` to a better place, 2025-09-08).

Let's adjust to that new schema.

Note: the `baseOwner` and `owners` attributes are left as-are, for the
transitional period until that PR and its friends are merged.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Sep 9, 2025
There was prior art that I should have used, as pointed out in
gitgitgadget#1991 (comment)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Member Author

dscho commented Sep 10, 2025

An aside while I was looking at this, I noticed CIHelper.parsePRURLInput() could be using getPullRequestKeyFromURL.

True! I have implemented that (still need to clean up the branch thicket, it needed fixes): dscho@8b3badb

Note to self: check whether default: ${{ vars.CONFIG }} works, and would avoid the need for the explicit config: ${{ vars.CONFIG }} lines added in gitgitgadget/gitgitgadget-workflows@2952485.

Okay, this does not work:

image
as text
Error: /home/runner/work/gitgitgadget/gitgitgadget/./check-run/action.yml (Line: 7, Col: 14): Unrecognized named-value: 'vars'. Located at position 1 within expression: vars.CONFIG
Error: Failed to load /home/runner/work/gitgitgadget/gitgitgadget/./check-run/action.yml

Sadness!

Even more sad is the excuse of a documentation of inputs.<input_id>.default which does not even mention that some contexts can be used, as some smarter people found out. But of course nothing official, because why would anyone want to know what they can use, and what not?</s>

dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 3, 2025
There was prior art that I should have used, as pointed out in
gitgitgadget#1991 (comment)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 6, 2025
There was prior art that I should have used, as pointed out in
gitgitgadget#1991 (comment)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 6, 2025
There was prior art that I should have used, as pointed out in
gitgitgadget#1991 (comment)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 6, 2025
There was prior art that I should have used, as pointed out in
gitgitgadget#1991 (comment)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to cygwingitgadget/gitgitgadget-workflows that referenced this pull request Oct 6, 2025
In gitgitgadget/gitgitgadget#1991, I adjusted
and augmented the `IConfig` interface a bit. Concretely, I renamed
`repo.baseOwner` to `repo.upstreamOwner` in c374c542 (IConfig: rename
the attribute defining the `upstream-repo`'s org, 2025-09-08) and moved
`repo.owners` to `app.installedOn` in 907807fb (IConfig: move
`repo.owners` to a better place, 2025-09-08).

Let's adjust to that new schema.

Note: the `baseOwner` and `owners` attributes are left as-are, for the
transitional period until that PR and its friends are merged.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 6, 2025
There was prior art that I should have used, as pointed out in
gitgitgadget#1991 (comment)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 7, 2025
There was prior art that I should have used, as pointed out in
gitgitgadget#1991 (comment)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho marked this pull request as ready for review October 7, 2025 06:35
@dscho dscho requested a review from mjcheetham October 7, 2025 06:35
@dscho
Copy link
Member Author

dscho commented Oct 7, 2025

@webstech would you kindly have a look?

@dscho dscho enabled auto-merge October 7, 2025 06:37
dscho added 10 commits October 7, 2025 09:19
In my endeavor to support projects other than Git, I am currently
adapting GitGitGadget to allow sending Cygwin PRs to the Cygwin-patches
mailing list.

I identified a couple of gaps in the project configuration when setting
up stuff in https://github.com/cygwingitgadget.

Let's close those gaps.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
We've settled on the nomenclature `upstream-repo` to refer to the
original repository of the project, as opposed to the `pr-repo` which is
a fork that exists exclusively to let GitGitGadget handle PRs in (and to
store its global state in the Git notes). So let's call the owner of the
`upstream-repo` the `upstreamOwner`, not the `baseOwner`.

Besides, with GitHub's naming conventions referring to the branch a PR
targets as the "base", it is a bit confusing to have `baseOwner` to
refer to anything except the owner of the repository in which the PR
lives.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The `owners` array refers to a list of orgs/owners where the GitHub App
is installed, i.e. where GitGitGadget can operate.

Therefore, a much better place is `app.installedOn`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The idea is to configure GitGitGadget via a `gitgitgadget-config.json`
file that contains the project-specific instance of the `IConfig`
interface, tracked in the `config` branch of a fork of the
`gitgitgadget-workflows` repository, from where it is automatically
synchronized via a GitHub workflow to the repository variable `CONFIG`,
and then passed to all of GitGitGadget's Actions via:

	```yml
	config: '${{ vars.CONFIG }}'
	```

For now, this input is optional, to ease the transition of GitGitGadget;
Eventually, this config will be required, so that several projects can
be served using identical source code in forks of the
`gitgitgadget-github-app` and `gitgitgadget-workflows` repositories.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
GitGitGadget now accepts the project configuration as a `config` Action
input, in the form of a string that contains the JSON-encoded `IConfig`
object. That is a bit fragile, though, as it is all-too-easy to have a
typo in that object.

Let's install `typia` to use the `IConfig` interface as a source of
truth when validating the user input.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This uses the freshly-installed `typia` module to create a validator for
the `IConfig` interface at compile-time, and uses it to validate
user-provided JSON against that interface.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
For the `typia`-based validator, it is good to label each and every
attribute's type so that the error messages are helpful.

This commit is best viewed with `--ignore-space-change`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This way, the maximal number of columns can be configured freely per
project, via the project-specific config.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Currently this URL is constructed from the `host` and the `name`
attributes of the project config setting's `mailrepo` attribute.

However, the `name` is supposed to refer to the mailing list _mirror
repository_, while we are interested in the URL where the web UI of the
public-inbox instance lives. Luckily, we already have that in the
project configuration: It's the `url` attribute.

I noticed the need for this patch in
cygwingitgadget/cygwin#1, where the URL
displayed after submitting v1 pointed to an incorrect location.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 7, 2025
There was prior art that I should have used, as pointed out in
gitgitgadget#1991 (comment)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
name: string; // name of the repo
owner: string; // owner of repo holding the notes (tracking data)
baseOwner: string; // owner of upstream ("base") repo
upstreamOwner: string; // owner of upstream ("base") repo
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment may be redundant now. It was probably originally trying to create the base to upstream relationship.

if (!newConfig.repo.baseOwner.match(/^[a-z\d](?:[a-z\d]|-(?=[a-z\d])){0,38}$/i)) {
throw new Error(`Invalid 'baseOwner' ${newConfig.repo.baseOwner} in ${filePath}`);
if (!newConfig.repo.upstreamOwner.match(/^[a-z\d](?:[a-z\d]|-(?=[a-z\d])){0,38}$/i)) {
throw new Error(`Invalid 'baseOwner' ${newConfig.repo.upstreamOwner} in ${filePath}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Change the error constant baseOwner to upstreamOwner?

const openPRCommits = (
await Promise.all(
this.config.repo.owners.map(async (repositoryOwner) => {
this.config.app.installedOn.map(async (repositoryOwner) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I agree with the naming but I won't disagree. Maybe it is authorized/allowed owners. Maybe I am still thinking about things still working without the app. Like many things in life, I can accept the name change.

As this gets implemented into other projects, changing the config will get more difficult.

@webstech
Copy link
Contributor

webstech commented Oct 9, 2025

fyi, review is in progress. Will remove this comment later.

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