-
Notifications
You must be signed in to change notification settings - Fork 97
Actions: start accepting a config
as input
#1991
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
base: main
Are you sure you want to change the base?
Conversation
lib/project-config.ts
Outdated
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 |
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.
@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
?
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 have no issue with renaming. My choice in names may not always be as git oriented as they could be.
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.
For the record, I am going forward with this: dscho@2de5309
lib/project-config.ts
Outdated
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) |
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.
@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...
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.
This is used several times in CIHelper
so the location could be justified. On the other hand, move it if you like.
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.
Here is the in-development patch to do so: dscho@9c2e794.
Note to self: check whether |
An aside while I was looking at this, I noticed |
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>
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>
True! I have implemented that (still need to clean up the branch thicket, it needed fixes): dscho@8b3badb
Okay, this does not work: as text
Sadness! Even more sad is the excuse of a documentation of |
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>
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>
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>
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>
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>
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>
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>
@webstech would you kindly have a look? |
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>
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>
lib/project-config.ts
Outdated
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 |
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.
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}`); |
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.
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) => { |
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.
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.
fyi, review is in progress. Will remove this comment later. |
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 thegitgitgadget-config.json
file in a dedicatedconfig
branch, from where it is synchronized via a GitHub workflow to the repository variable calledCONFIG
.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 orcurl
'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