-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
BOTMETA.yml: remove superfluous labels #44628
BOTMETA.yml: remove superfluous labels #44628
Conversation
@@ -1227,18 +1213,15 @@ files: | |||
test/integration/targets/aci: | |||
maintainers: $team_aci | |||
labels: | |||
- aci |
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 incorrect, we are matching a substring here.
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.
As far as i know, botmeta handles substrings like a whole path: aci
is a path component of test/integration/targets/aci
. There are four path components in test/integration/targets/aci
: test
, integration
, targets
and aci
, test
and aci
are valid GitHub labels, only this ones are kept.
I run manually ansibullbot with this diff on 43847:
diff --git a/ansibullbot/parsers/botmetadata.py b/ansibullbot/parsers/botmetadata.py
index 7faa674..3dd94e4 100644
--- a/ansibullbot/parsers/botmetadata.py
+++ b/ansibullbot/parsers/botmetadata.py
@@ -67,6 +67,8 @@ class BotMetadataParser(object):
def extend_labels(data):
for k, v in data['files'].items():
+ if k == "test/integration/targets/aci":
+ v.pop('labels')
# labels from path(s)
if v is None:
continue
self.meta.get('component_labels') is still equal to ['aci', 'test']
.
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.
@pilou- Hmm, alright, I would have expected it to first match the path, and only then use path-components for matching labels. Because in our case, the paths would not be test/integration/targets/aci, but rather test/integration/target/aci_rest/ and others... So I was expecting it to match aci_rest
, and not aci
.
PS That's why I prefer to use a trailing slash if it's known that the path is a directory and not a substring match.
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.
So should I update these entries keeping labels and adding an underscore (for example: test/integration/target/aci_
) ?
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.
That's a possibility, but I don't know if this is the same as was intended. (I.e. it is possible that the targets do not reflect modules per se). I'd rather add trailing slashes in those cases where it is a directory and leave it as is. Thanks for clarifying on how Ansibot works ;-)
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.
Ok, rebased :)
- networking | ||
test/integration/targets/meraki: | ||
maintainers: $team_meraki | ||
labels: | ||
- meraki |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- networking | ||
test/integration/targets/nxos: | ||
maintainers: $team_nxos | ||
labels: | ||
- networking | ||
- nxos |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
@@ -1247,17 +1230,14 @@ files: | |||
maintainers: $team_ucs | |||
labels: | |||
- remote_management | |||
- ucs |
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 incorrect, we are matching a substring here.
Please merge #44574 first... |
@pilou- If you fix the conflict, we can merge this. |
Path components and filename without extension which are valid GitHub labels are already default labels.
cc6e58d
to
73eaf8d
Compare
* devel: (513 commits) Fix systemd service is already masked issue (#44730) fix issue with no_log in py3 modules/terraform: Quote the variable values in the command line (#43493) YUM4/DNF compatibility via yum action plugin (#44322) BOTMETA.yml: remove superfluous labels (#44628) Share the implementation of hashing for both vars_prompt and password_hash (#21215) one_host environment variables, Fixes #44163 (#44568) ec2: add "IAM Role" to instance_profile_name ios_vrf speed fix (#43765) fix typo (#44712) junos cli_config idempotence fix (#44706) Switch to LiteralPath instead of Path. Closes #44508 (#44509) Module win_domain_computer fix delete computer with child (#44500) ACME: improve documentation (#44691) doc: fixed typo (#44685) IPA: Add option to specify timeout (#44572) Added nios_txt_record module (#39264) adds the bigip_cli_script module (#44674) Clean up BOTMETA.yml (#44574) Change validate-modules for removed modules ...
SUMMARY
Following #44574: Path components and filename without extension which are valid GitHub labels are already default labels.
ISSUE TYPE
COMPONENT NAME
.github/BOTMETA.yml
ANSIBLE VERSION