Add custom properties#2512
Conversation
Sandijigs
requested review from
Mark-Simulacrum,
emilyalbini,
jdno,
marcoieni and
ubiratansoares
as code owners
|
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 |
jieyouxu
added
needs-infra-admin-review
ubiratansoares
left a comment
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 :)
| let operation = match actual_by_name.get(name) { | ||
| // Missing on the repo, or value is null. | ||
| None | Some(None) => CustomPropertyDiffOperation::Create(expected), | ||
| Some(Some(actual)) if actual != &expected => { | ||
| CustomPropertyDiffOperation::Update(actual.clone(), expected) | ||
| } | ||
| Some(Some(_)) => continue, | ||
| }; |
There was a problem hiding this comment.
Perhaps is room to make this code more concise with some pattern matching tricks. I'd recommend having a look on how to change
let operation = match actual_by_name.get(name)
in order to get rid of the nested Option value :)
| "auto_merge_enabled": true | ||
| "auto_merge_enabled": true, | ||
| "custom_properties": {} | ||
| } No newline at end of file |
There was a problem hiding this comment.
It's probaly a good idea adding an empty line in these files, so we keep the standard
| Ok(ruleset_diffs) | ||
| } | ||
|
|
||
| async fn diff_custom_properties( |
There was a problem hiding this comment.
I missed some tests that actually prove this function works as intended in sync/github/tests/mod.rs.
I propose adding at least two cases :
- adding a propery to a repo
- removing a property from a repo
| // 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)] | ||
| pub custom_properties: BTreeMap<String, bool>, |
There was a problem hiding this comment.
In general we've been using IndexMap to represent key-value pairs in this file. Do you have any particular reason to go with BTreeMap instead?
| // 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).
| #[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.
| # 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?
5 participants
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.