Skip to content
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

Allow to pass ES_TIME_RANGE var to Spark dependencies job #1481

Merged
merged 2 commits into from
Jun 28, 2021

Conversation

Gr1N
Copy link
Contributor

@Gr1N Gr1N commented Jun 22, 2021

Which problem is this PR solving?

Short description of the changes

  • added elasticsearchTimeRange configuration option for Jaeger dependencies section

@Gr1N
Copy link
Contributor Author

Gr1N commented Jun 23, 2021

@jpkrohling @kevinearls review please

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jpkrohling jpkrohling enabled auto-merge (squash) June 24, 2021 13:32
auto-merge was automatically disabled June 24, 2021 13:57

Head branch was pushed to by a user without write access

@Gr1N Gr1N requested a review from jpkrohling June 24, 2021 13:59
@Gr1N
Copy link
Contributor Author

Gr1N commented Jun 24, 2021

@jpkrohling please review again, I've fixed formatting

@jpkrohling jpkrohling enabled auto-merge (squash) June 24, 2021 14:30
Signed-off-by: Nikita Grishko <gr1n@protonmail.com>
auto-merge was automatically disabled June 24, 2021 15:07

Head branch was pushed to by a user without write access

@Gr1N
Copy link
Contributor Author

Gr1N commented Jun 24, 2021

@jpkrohling I'm sorry but can you please check PR again? This time I walked through contributing guide and ran everything except e2e tests. Hope everything is ok now.

@Gr1N Gr1N requested a review from jpkrohling June 24, 2021 15:09
@codecov
Copy link

codecov bot commented Jun 24, 2021

Codecov Report

Merging #1481 (6e71fb2) into master (b9d1e02) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1481   +/-   ##
=======================================
  Coverage   87.20%   87.21%           
=======================================
  Files          90       90           
  Lines        5003     5004    +1     
=======================================
+ Hits         4363     4364    +1     
  Misses        484      484           
  Partials      156      156           
Impacted Files Coverage Δ
pkg/apis/jaegertracing/v1/jaeger_types.go 100.00% <ø> (ø)
pkg/cronjob/spark_dependencies.go 93.26% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9d1e02...6e71fb2. Read the comment docs.

@jpkrohling jpkrohling enabled auto-merge (squash) June 25, 2021 12:30
@rubenvp8510
Copy link
Collaborator

I don't think the e2e failing is related to the changes made in this PR.

There is an issue with the ingress test, which should be fixed by this PR: #1491 As soon as that PR is merged could you rebase?

@Gr1N
Copy link
Contributor Author

Gr1N commented Jun 26, 2021

@rubenvp8510 yep, no problem

@jpkrohling
Copy link
Contributor

PR rebased.

@jpkrohling jpkrohling merged commit bced782 into jaegertracing:master Jun 28, 2021
@Gr1N Gr1N deleted the deps-es-time-range branch June 28, 2021 15:59
@Gr1N
Copy link
Contributor Author

Gr1N commented Jun 28, 2021

@jpkrohling thank you for the help and maybe you know when new version of the operator will be available to use?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants