-
Notifications
You must be signed in to change notification settings - Fork 141
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
SHOW CATALOGS documentation and integ tests #977
SHOW CATALOGS documentation and integ tests #977
Conversation
3ba5730
to
86eb22d
Compare
task startPrometheus(type: SpawnProcessTask) { | ||
doFirst { | ||
download.run { | ||
src 'https://github.com/prometheus/prometheus/releases/download/v2.39.1/prometheus-2.39.1.linux-amd64.tar.gz' |
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.
What is the best practice here? Should I rely on github links or S3 links.
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.
wondering how often this needs to be changed?
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.
Till the next major release. I will be sticking to this version.
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.
should this test be optional? I assume it won't work on arm64 and windows/mac
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 have seen derek's PR, it removed doctests and integTests for other platforms. When we fix for them we can use mac/windows binaries for them.
or we can create a separate task including tests for prometheus.
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.
yes that's ok then, the integTest/docTests should be fixed when 2.4 is out, then this can be changed to support other platforms
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.
Created an issue for supporting mac/windows os types. #982
9981c5f
to
89b65a6
Compare
Codecov Report
@@ Coverage Diff @@
## 2.x #977 +/- ##
=============================================
- Coverage 97.55% 62.76% -34.80%
=============================================
Files 303 10 -293
Lines 7815 658 -7157
Branches 507 119 -388
=============================================
- Hits 7624 413 -7211
- Misses 190 192 +2
- Partials 1 53 +52
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
1fe3c91
to
b2bbe38
Compare
b2bbe38
to
d513fc5
Compare
task startPrometheus(type: SpawnProcessTask) { | ||
doFirst { | ||
download.run { | ||
src 'https://github.com/prometheus/prometheus/releases/download/v2.39.1/prometheus-2.39.1.linux-amd64.tar.gz' |
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.
should this test be optional? I assume it won't work on arm64 and windows/mac
} | ||
copy { | ||
from "$projectDir/bin/prometheus.yml" | ||
into "$projectDir/bin/prometheus-2.39.1.linux-amd64/prometheus" |
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.
minor issue, maybe rename prometheus-<version>.<platform>-<arch>
to prometheus
so the directory name is fixed? or bin/prometheus-*
, not sure if *
is supported
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.
tar file gets unzipped into prometheus-2.39.1.linux-amd64. Need to introduce an unnecessary copy step in between to prometheus.
prometheus-2.39.1.linux-amd64
will remain for sometime. I don't have plans to change this in near term. If one changes the tar link, he can make change to folder name as well.
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.
seems there's no easy way to do --strip-component
in gradle.. this needs to change when other platforms support is added, i think it's ok to change then, maybe use a constant
Signed-off-by: Vamsi Manohar <reddyvam@amazon.com>
d513fc5
to
69bfc64
Compare
Signed-off-by: Vamsi Manohar <reddyvam@amazon.com>
Signed-off-by: Vamsi Manohar reddyvam@amazon.com
Description
Covers integration tests, doc tests and documentation for #925
Issues Resolved
[List any issues this PR will resolve]
Check List
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.