-
Notifications
You must be signed in to change notification settings - Fork 127
Update coverage information with tags and transforms #2831
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
base: main
Are you sure you want to change the base?
Conversation
test integrations |
Include coverage about kibana tags, and ignore the validations config file.
cb5fcbc
to
9583040
Compare
test integrations |
Created or updated PR in integrations repository to test this version. Check elastic/integrations#15143 |
test integrations |
Created or updated PR in integrations repository to test this version. Check elastic/integrations#15143 |
Tag detection for tags in |
/test |
Looking at the packages that have failed those
Could it be checked if there are Kibana assets in the directory first ? However, there are two packages that they have kibana assets and a In case of It looks like that tags created by fleet have a tooltip Would it be ok to filter the packages with the description somehow when validating the kibana/tags.yml? |
This is expected, isn't it? Tags defined in
Yes, I haven't managed to make it fail locally, and in CI it sometimes works, even here we have now a green check. I am starting to think that these tags are installed asynchronously, and then we may have some kind of race condition.
Yeah, a bit confusing I think, because the tag is not really defined in the spec. Also looking to how ids are generated we cannot trust on them being just lowercased versions of the names, this only works for the security tag, others contain random uuids.
I think the problem is that they are not there yet, so we would need to introduce some kind of wait until mechanism in any case. So I think that we would need to do at least:
Apart from that, once we have the wait mechanism, we could check for the rest of assets to be actually present. Maybe I could split this PR in two, here I only mark the files as covered, and in a follow up we add the other enhancements, wdyt? |
Doing this in dadf441, I will leave further improvements for future changes. |
You're right, they do not have to be defined as assets. But in those packages that I mentioned (e.g. azure_blob_storage) there are no Kibana assets. And the tags from |
That scenario looks likely to happen 👍 .
It sounds a good plan to follow. But I still think that tags defined in |
You are right, fixing this and adding test case in 039273f. |
// tags defined in tags.yml, whose id can be unpredictable, so check by name. | ||
if len(actualAssets) == 0 { | ||
// If there are no assets, the tag may not be installed, so assume it would have been. | ||
// TODO: More accurately we should check if any of the listed tags in `tags.yml` is present. |
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 would be actually wrong in the package, and can require quite some changes to be supported as we would need to bring here the whole tags.yml
. So if we find a package with this I would prefer to fix this in the package.
test integrations |
Created or updated PR in integrations repository to test this version. Check elastic/integrations#15143 |
test integrations |
Created or updated PR in integrations repository to test this version. Check elastic/integrations#15143 |
test integrations |
Created or updated PR in integrations repository to test this version. Check elastic/integrations#15143 |
Moving back to draft, there are still cases causing problems with tags. |
Putting this on hold as this will require more changes than expected. |
⏳ Build in-progress, with failures
Failed CI StepsHistory
cc @jsoriano |
Related to #2704.
Add some information for coverage reports:
validation.yml
is ignored.