-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add new fields to integration test and distribution build publish libraries #496
Conversation
15318c9
to
a284fbd
Compare
component_repo
and component_repo_url
fieldsa284fbd
to
ddda5cd
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #496 +/- ##
=========================================
Coverage 84.32% 84.32%
Complexity 80 80
=========================================
Files 108 108
Lines 523 523
Branches 61 61
=========================================
Hits 441 441
Misses 26 26
Partials 56 56 ☔ View full report in Codecov by Sentry. |
a4af87d
to
e30b297
Compare
e30b297
to
d88c840
Compare
0e42cce
to
2ccf852
Compare
Testing, following is the generate sample JSON documents which has all the added new fieldsIntegration test results
Distribution build results
|
vars/publishIntegTestResults.groovy
Outdated
@@ -54,25 +54,32 @@ void call(Map args = [:]) { | |||
|
|||
manifest.components.each { component -> | |||
def componentName = component.name | |||
def componentRepo = component.repository.split('/')[-1].replace('.git', '') | |||
def componentRepoUrl = component.repository |
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.
I think we should also remove .git
here as sometimes people didnt add that in input manifest, causing bundle manifest to sometimes have .git sometimes not, which will make your data diverse for the same component at times.
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.
I feel like we should also try to remove everything before github.com
as people tend to mistype https vs http or anything, causing diverse data.
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 component repo url should just show opensearch-project/OpenSearch
.
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.
Got It you mean https://github.com/opensearch-project/{component_name}
, but this would cause problems in long term if we have to build and publish plugins from other org apart from opensearch-project
. Also @peterzhuamazon with your automation the manifests are now auto generated from past release manifests so technically no errors and anyhow we review for new plugins that are added. Also if merged with errors and fixed later the new documents indexed will have the updated value for the component_repo_url
.
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.
No, just strip https://
http://
.git
and then index.
You dont need to hardcode the opensearch-project.
Index as github.com/opensearch-project/OpenSearch
or similar.
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.
I was thinking index as https://github.com/opensearch-project/OpenSearch
and using OSD scripted fields just add opensearch-project/OpenSearch
to reduce the field length for visualizations, but during any query it would return the full URL as https://github.com/opensearch-project/OpenSearch
.
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.
does it matter?
url with or without .git
takes you to the same page.
Also, didn't get the reason behind dropping https
?
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.
Its because Rishab, when we browse (just try copy pasting this in browser) github.com/opensearch-project/OpenSearch
, it should add https://
and add take us to the repo.
2ccf852
to
2714a73
Compare
I can see the following after the latest commit, please check Integration test publish:
Distribution build publish:
|
Signed-off-by: Prudhvi Godithi <[email protected]>
2714a73
to
cb4c52e
Compare
This PR will share the same version: Thanks. |
…raries (#496) Signed-off-by: Prudhvi Godithi <[email protected]> (cherry picked from commit 5cc8d08) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
component_repo
andcomponent_repo_url
fields for both the distribution and integration test publish libs. With these fields all the distribution and integration test documents can be filtered bycomponent_repo
(orcomponent_repo_url
).component_repo
should give the all the CI groups test failures.test_stdout
andtest_stderr
for bothwith-security
andwithout-security
should give users direct downloads links for the stderr and stdout errors. Thanks to @peterzhuamazon coming from Add test_stdout and test_stderr path/url for test-report.yml opensearch-build#5001.Issues Resolved
Part of opensearch-project/opensearch-metrics#72 and opensearch-project/opensearch-metrics#51
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.