Add custom properties#2512
Conversation
|
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 |
Dry-run check results |
ubiratansoares
left a comment
There was a problem hiding this comment.
@Sandijigs thanks for this PR, looks we are in the right direction here. Added a few comments :)
| // 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)] |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Removed. Makes sense given the generator updates before consumers.
| #[derive(Debug)] | ||
| enum CustomPropertyDiffOperation { | ||
| Create(String), | ||
| Update(String, String), // old, new |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll check whether there are any custom properties out there, so we don't break anything by mistake here
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.. :) ).
| # Repository custom properties (optional) | ||
| [custom-properties] | ||
| # Set a property name to a boolean value | ||
| crabwatch = true |
There was a problem hiding this comment.
The property values on GitHub are strings, and the code currently converts bools to a string via its Display impl, which is a bit opaque and potentially fragile. Maybe we could just expose the value as a string?
There was a problem hiding this comment.
That's a fair point. The reason I went with bool is that @marcoieni suggested it earlier. TOML supports it natively, and the conversion to string happens on the Rust side.
I can switch to strings throughout if you'd rather, but probably worthchecking with @marcoieni first so we don't end up bouncing between the two.
There was a problem hiding this comment.
Yeah, I was wrong on this. The fact that the property crabwatch can be only true or false isn't true for all properties, so it makes sense to use strings. Sorry about this!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2a53766 to
ef1724a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ubiratansoares
left a comment
There was a problem hiding this comment.
@Sandijigs looks good to me, one small thing from my end
3dfeadd to
a9b0499
Compare
|
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. |
Adds support for
[custom-properties]in the team repo's TOML, and setscrabwatch = trueon rust-lang/crabwatch.Booleans only for now. Properties on a repo but not declared in TOML are left alone.
Closes #2504.