Skip to content

Add custom properties#2512

Merged
marcoieni merged 2 commits into
rust-lang:mainfrom
Sandijigs:add-custom-properties
Jul 7, 2026
Merged

Add custom properties#2512
marcoieni merged 2 commits into
rust-lang:mainfrom
Sandijigs:add-custom-properties

Conversation

@Sandijigs

Copy link
Copy Markdown
Contributor

Adds support for [custom-properties] in the team repo's TOML, and sets crabwatch = true on rust-lang/crabwatch.
Booleans only for now. Properties on a repo but not declared in TOML are left alone.
Closes #2504.

@rustbot

rustbot commented Jun 9, 2026

Copy link
Copy Markdown

rust_team_data/src/v1.rs has been modified, it is used (as a git dependency) by multiple sub-projects like triagebot, the www.rust-lang.org website and others.

If you are changing the data structures, please make sure that the changes are not going to break serde deserialization (adding a field is fine; removing or renaming a field isn't).

If you must do a breaking change to the format, make sure to coordinate it with all the users of the rust_team_data crate.

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Dry-run check results

[WARN  rust_team::sync] sync-team is running in dry mode, no changes will be applied.
[INFO  rust_team::sync] synchronizing crates-io
[INFO  rust_team::sync] synchronizing github
@jieyouxu jieyouxu added needs-infra-admin-review This change requires one of the `infra-admins` to review. S-waiting-on-review Status: waiting on review from a team/WG/PG lead, an infra-admin, and/or a team-repo-admin. labels Jun 10, 2026

@ubiratansoares ubiratansoares left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Sandijigs thanks for this PR, looks we are in the right direction here. Added a few comments :)

Comment thread src/sync/github/mod.rs Outdated
Comment thread tests/static-api/_expected/v1/repos/archived_repo.json Outdated
Comment thread src/sync/github/mod.rs
Comment thread rust_team_data/src/v1.rs Outdated

@Kobzol Kobzol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left a few comments :)

Comment thread rust_team_data/src/v1.rs Outdated
// Is the GitHub "Auto-merge" option enabled?
// https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request
pub auto_merge_enabled: bool,
#[serde(default)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The #[serde(default)] annotation is not necessary, because the generator (the team API CI) will be updated sooner than the consumers (the crates that depend on the team crate).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed. Makes sense given the generator updates before consumers.

Comment thread src/sync/github/mod.rs Outdated
#[derive(Debug)]
enum CustomPropertyDiffOperation {
Create(String),
Update(String, String), // old, new

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should perhaps also contain Delete, in case the custom property is removed from the team config.

It would mean that team is the ground truth for all our custom properties, although it would also delete all custom properties that are not set in team.

@ubiratansoares Do you want team to be the ground truth for all custom properties? If it is not, then once a property is created through team, it won't ever be deleted again.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you want team to be the ground truth for all custom properties?

That's an interesting question, my gut feeling says yes, since I see these custom properties as tool to enable governance-related automations over Github repos or Github orgs, and team is the place where we want represent and enable such stuff with IaC.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree, but we should be then careful with the first sync (or ideally a dry-run executed manually) to ensure that we don't delete any existing custom properties; those would have to be backported into team first.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll check whether there are any custom properties out there, so we don't break anything by mistake here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the Delete variant. The diff emits a. Delete for any property on GitHub that isn't declared in TOML, so team becomes the source of truth. The apply sends a null value through the existing PATCH endpoint, which is GitHub's way of unsetting.

@ubiratansoares ubiratansoares Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll check whether there are any custom properties out there, so we don't break anything by mistake

@Sandijigs @Kobzol just to confirm : I ran a custom script and confirmed that no repos actually use custom properties (for rust-lang org)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, good to know. In that case we can directly switch to using team as a source of truth (unless repos in some of our other orgs use them.. :) ).

Comment thread docs/toml-schema.md
@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@Sandijigs Sandijigs force-pushed the add-custom-properties branch from 2a53766 to ef1724a Compare June 21, 2026 18:10
@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@ubiratansoares ubiratansoares left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Sandijigs looks good to me, one small thing from my end

Comment thread docs/toml-schema.md Outdated
@Sandijigs Sandijigs force-pushed the add-custom-properties branch from 3dfeadd to a9b0499 Compare June 22, 2026 21:13
@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@ubiratansoares ubiratansoares left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Sandijigs Some additional considerations after trying out custom properties in a org/repo I own.

Comment thread src/sync/github/api/mod.rs Outdated
#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
pub(crate) struct CustomPropertyValue {
pub(crate) property_name: String,
pub(crate) value: Option<String>,

@ubiratansoares ubiratansoares Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right now we are modelling propertie values as optional string fields, but actually what we get here in the API response will depend on specifics of custom property settings:

Image

As an example, when we set the property type as multi-select we'll get an array of strings in the API response. Tested with one my repos:

gh api \
  -H "Accept: application/vnd.github+json" \
  -H "X-GitHub-Api-Version: 2026-03-10" \
  /repos/dotanuki-labs/testbed/properties/values

[
  {
    "property_name": "Category",
    "value": [
      "DevOps"
    ]
  }
]
Image

Perhaps we don't need to support all types of custom properties to get started, but we must clarify that we support only text-field like properties for now

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

for this I kept as Option for now and updated the schema docs to note only text-type properties are supported.

Values are stored as strings. Only text-type custom properties are supported for now.

Comment on lines +711 to +712
if !self.dry_run {
// REST API: PATCH /repos/{owner}/{repo}/properties/values

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since custom properties must be pre-defined at org-level to be applied, I think we should capture whether we are able to apply the custom property in the dry-run

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the check. When a TOML property isn't defined at the org level, the diff emits a CannotApply operation and the apply step skips the API call.

team/src/sync/github/mod.rs

Lines 927 to 935 in dc12bfb

if !org_property_names.contains(name) {
diffs.push(CustomPropertyDiff {
name: name.clone(),
operation: CustomPropertyDiffOperation::CannotApply {
reason: format!("'{name}' is not defined at the org level"),
},
});
continue;
}

Comment thread docs/toml-schema.md Outdated
@Sandijigs Sandijigs force-pushed the add-custom-properties branch from a9b0499 to 40e3c1c Compare July 2, 2026 18:27
@rustbot

This comment has been minimized.

@Sandijigs Sandijigs force-pushed the add-custom-properties branch from e446f66 to dc12bfb Compare July 3, 2026 08:30
@rustbot

This comment has been minimized.

@marcoieni marcoieni assigned marcoieni and unassigned ubiratansoares Jul 3, 2026
Comment thread docs/toml-schema.md Outdated
Comment thread docs/toml-schema.md Outdated

[Repository custom properties] are values set on a repository to opt it into org-wide tooling. The property must first be defined at the organization level.

Values are stored as strings. Only text-type custom properties are supported for now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Values are stored as strings. Only text-type custom properties are supported for now.
Only text-type custom properties are supported for now.

Remove redundant implementation detail

Comment thread docs/toml-schema.md Outdated
@marcoieni

marcoieni commented Jul 4, 2026

Copy link
Copy Markdown
Member

we should at least support also booleans additionally to strings, so that we can define the crabwatch label as a boolean from the beginning, so that we don't have to do migrations later.

Supporting only booleans is also ok.

I can work on this if you want.

EDIT: I asked gpt 5.5 to implement it and it looks good. I committed these changes.

@marcoieni

Copy link
Copy Markdown
Member

I created the custom property for the rust-lang org
image

@Sandijigs

Copy link
Copy Markdown
Contributor Author

I created the custom property for the rust-lang org image

Thanks for pushing this through. I've pulled the changes locally and tests pass. Reading through the diff now to make sure I understand the design.

@Sandijigs Sandijigs closed this Jul 5, 2026
@Sandijigs Sandijigs reopened this Jul 5, 2026
@marcoieni

Copy link
Copy Markdown
Member

I think this is ready to be merged. Can you reorganize your commits in a way that you like? If you prefer, I can squash them into a single one, too.

@Sandijigs

Copy link
Copy Markdown
Contributor Author

Great, I will squash all commits into a single one

@Sandijigs Sandijigs force-pushed the add-custom-properties branch from a965663 to 5f12c47 Compare July 6, 2026 19:52
@rustbot

rustbot commented Jul 6, 2026

Copy link
Copy Markdown

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@Sandijigs

Copy link
Copy Markdown
Contributor Author

It has been squashed to a single commit. Ready to merge.

marcoieni
marcoieni previously approved these changes Jul 7, 2026
@marcoieni marcoieni added this pull request to the merge queue Jul 7, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 7, 2026
@marcoieni

marcoieni commented Jul 7, 2026

Copy link
Copy Markdown
Member

apparently github api doesn't support booleans for custom properties 😅 I will make some changes quickly, then try to merge again.

@marcoieni marcoieni enabled auto-merge July 7, 2026 07:03
@marcoieni marcoieni added this pull request to the merge queue Jul 7, 2026
Merged via the queue into rust-lang:main with commit 54d26d9 Jul 7, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-infra-admin-review This change requires one of the `infra-admins` to review. S-waiting-on-review Status: waiting on review from a team/WG/PG lead, an infra-admin, and/or a team-repo-admin.

6 participants