feat(labels): support exclusion regexp - #408
Conversation
|
ping @decyjphr @martinm82 Does this make sense? It occurred to me that the plugin is given the object from the config at the plugin level and not the whole config so instead of having a separate root config node it felt it made more sense to keep it in the labels plugin configuration rather than as a separate config key like I mentioned in the discussion thread. It's only to exclude labels from being deleted but I thought using the Other options considered and scrapped: Note: Unintentional white-space update of the README by my editor, let me know if you want me to redo the documentation update without the white-space update. |
6fb6f71 to
9de8e1a
Compare
|
Maybe an unexpected side-effect depending on the config values: labels:
exclude:
- .
- '[^]' |
This is in my opinion a very nice addition and we should have this feature as well on topics (can be a separate PR if preferred). WDYT? |
|
I took the liberty to create a separate feature request for your suggestion about topics: #411 |
|
@anderssonjohan what do you think about documenting it in the README? |
|
I was thinking a bit more about this and I am wondering whether the default behaviour should be to keep labels/topics and update with the ones managed in safe-settings. In our case we have 9k repositories and started just recently using safe-settings. I cannot manage labels on org level since as of now it will delete all manually created ones. I believe that having the "do not delete" behaviour as default makes the transition to safe-settings much safer. WDYT? Also, would be good to spit out in debug logs those labels that are not tracked by safe-settings. Later teams can get those from the logs (splunk, dataset, Loki, etc.) and migrate them over to safe-settings. |
@martinm82 Do you mean documenting both syntax options? The (non-whitespace) change I made to the README in this PR was modifying the sample to use include/exclude for labels: https://github.com/github/safe-settings/pull/408/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5L325 |
|
@anderssonjohan this is a cool (and valuable) feature. Thank you. I'll test it and merge it. |
Sorry, I missed that between all the whitespaces changes. |
I think Johan is about to provide an updated version without the whitespace changes, so I'd wait for that before merging 🙂 |
Adds support for alternative labels configuration where
labels matching a given pattern can be excluded from
delete operations. This allows other tools to create
labels without having them deleted by safe-settings.
Example: The following config would keep the labels
`released`, `released on @dev`, `released on @7.x`
created by a tool such as Semantic Release.
```
labels:
include:
- name: bug
color: '#ff0000'
exclude:
- name: ^release
```
Include can be used without specifying exclude in the
config, while also supporting the current format:
```
labels:
- name: bug
color: '#ff0000'
```
9de8e1a to
70d1c95
Compare
|
Sorry for the late response to this. Now ready for merge @decyjphr Thank you @adrianschmidt for pointing out that two trailing spaces means hard newline in Markdown! Anyhow, readme now only have the intended change for the labels example. EDIT: For the one also bitten by the VSCode formatter: microsoft/vscode#1679 (comment) |
|
@anderssonjohan for the existing configurations, Would this be a breaking change, i.e. they have to modify the |
@decyjphr Thanks for asking! it's backwards compatible by checking if the include is an array or not here: https://github.com/github/safe-settings/pull/408/files#diff-e71ad8761eefa8f71d7f5c23541f2078574ae83d48423be0d06dd16740d43cc3R8 |
Actually, you can easily get this default behaviour by adding the exclude in the org settings. |
|
@anderssonjohan I have been playing around with the exclude pattern (#408 (comment)) and it seems that if a repo is already managed with safe-settings and you create a label manually the first time it gets still deleted. The next time you create it again, safe-settings does not touch it again. |
No, sorry, nothing that I've seen. Are you sure it got the webhook notification etc the second time? |
…d property values Custom properties cannot be deleted through the API, so Diffable's removal path sets `value: null`. Any property present on a repository but absent from config was therefore cleared on every sync, including values written by other automation. Safe-settings effectively claimed exclusive ownership of the whole custom-properties surface on any repo with a `custom_properties` section. Adds the `include`/`exclude` config shape already used by the Labels plugin (github-community-projects#408). Properties matching an `exclude` regex are never cleared; properties listed in `include` are enforced exactly as before, even if they also match an exclude pattern. The plain-array config shape is unchanged. Unlike Labels, an unparseable exclude regex is recorded as a per-repo config error and skipped rather than thrown. Child plugins are constructed outside any try/catch in Settings.updateRepos, so a throw there rejects the org-wide Promise.all and aborts the sync for every other repo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d property values Custom properties cannot be deleted through the API, so Diffable's removal path sets `value: null`. Any property present on a repository but absent from config was therefore cleared on every sync, including values written by other automation. Safe-settings effectively claimed exclusive ownership of the whole custom-properties surface on any repo with a `custom_properties` section. Adds the `include`/`exclude` config shape already used by the Labels plugin (github-community-projects#408). Properties matching an `exclude` regex are never cleared; properties listed in `include` are enforced exactly as before, even if they also match an exclude pattern. The plain-array config shape is unchanged. Both config-error paths are designed to fail safe, because the cost of misreading an exclude pattern is cleared property values: * Exclude patterns are lowercased when compiled. Property names are lowercased before matching, so an uppercase pattern would otherwise be a valid regex that silently matches nothing and clears the properties it was written to protect. * An unparseable pattern is recorded as a per-repo config error and fails closed - every property on that repo is treated as excluded, so a typo protects values rather than clearing them. This matters because these are regexes, not globs: the intuitive `*` is not a valid pattern. Properties in `include` are still applied. The error is not thrown, because child plugins are constructed outside any try/catch in Settings.updateRepos, so a throw there rejects the org-wide Promise.all and aborts the sync for every other repo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d property values Custom properties cannot be deleted through the API, so Diffable's removal path sets `value: null`. Any property present on a repository but absent from config was therefore cleared on every sync, including values written by other automation. Safe-settings effectively claimed exclusive ownership of the whole custom-properties surface on any repo with a `custom_properties` section. Adds the `include`/`exclude` config shape already used by the Labels plugin (github-community-projects#408). Properties matching an `exclude` regex are never cleared; properties listed in `include` are enforced exactly as before, even if they also match an exclude pattern. The plain-array config shape is unchanged. Both config-error paths are designed to fail safe, because the cost of misreading an exclude pattern is cleared property values: * Exclude patterns are lowercased when compiled. Property names are lowercased before matching, so an uppercase pattern would otherwise be a valid regex that silently matches nothing and clears the properties it was written to protect. * An unparseable pattern is recorded as a per-repo config error and fails closed - every property on that repo is treated as excluded, so a typo protects values rather than clearing them. This matters because these are regexes, not globs: the intuitive `*` is not a valid pattern. Properties in `include` are still applied. The error is not thrown, because child plugins are constructed outside any try/catch in Settings.updateRepos, so a throw there rejects the org-wide Promise.all and aborts the sync for every other repo. * The object form requires `include`, `exclude`, or both. A config that is neither the array shape nor the include/exclude shape (`custom_properties: {}`, say) is reported and also fails closed. The schemas reject that shape too, but they are authoring aids rather than a runtime gate - nothing in lib/ validates config against them - so the plugin does not rely on them. Without this the empty object reached `normalizeEntries` and threw a TypeError out of the constructor, which is the org-wide abort described above. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4 participants
fixes #407
fixes #398
Adds support for alternative
labelsconfiguration where labels matching a given pattern can be excluded from delete operations. This allows other tools to create labels without having them deleted by safe-settings.Example: The following config would keep the labels
released,released on @dev,released on @7.xcreated by a tool such as Semantic Release.Include can be used without specifying exclude in the config, while also supporting the current format: