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

Update UI documentation link if is present #1290

Merged
merged 1 commit into from
Nov 19, 2020

Conversation

rubenvp8510
Copy link
Collaborator

@rubenvp8510 rubenvp8510 commented Nov 3, 2020

Fixes #1292

Signed-off-by: Ruben Vargas Palma [email protected]

@rubenvp8510 rubenvp8510 changed the title Update UI documentation link if it's present Update UI documentation link if is present Nov 3, 2020
@codecov
Copy link

codecov bot commented Nov 3, 2020

Codecov Report

Merging #1290 into master will decrease coverage by 0.02%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1290      +/-   ##
==========================================
- Coverage   87.32%   87.30%   -0.03%     
==========================================
  Files          89       89              
  Lines        4947     4970      +23     
==========================================
+ Hits         4320     4339      +19     
- Misses        464      466       +2     
- Partials      163      165       +2     
Impacted Files Coverage Δ
pkg/strategy/controller.go 93.33% <75.00%> (-1.74%) ⬇️

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 c418931...bf0fcfa. Read the comment docs.

if !ok {
return false, 0
}
if itemLabel, ok := itemMap["label"]; ok && itemLabel == "Documentation" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we set this initially, or is it only set by users? Can we somehow check whether the value was set by us? We should really avoid changing a value that was set by users.

Copy link
Contributor

Choose a reason for hiding this comment

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

As this issue is related to the doc URL being supplied via CLI flag, can we just provide a fix for that situation (i.e. check if the CLI flag has been defined)?

Copy link
Collaborator Author

@rubenvp8510 rubenvp8510 Nov 4, 2020

Choose a reason for hiding this comment

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

@jpkrohling TBH I don't like this either, unfortunately there is not a way to know if this was set by us or, not that I'm aware of. :(

@objectiser This code is only executed if the documentation flag is set, See: https://github.com/jaegertracing/jaeger-operator/pull/1290/files#diff-8be2eee66bfcb8efd08e4b1fe2795663a53935ff150187ad426d371534045b01R258

The update of the doc will be only executed if the CLI flag is set, there is something on the menu, and the menu structure and labels match.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jpkrohling In this situation, if the doc URL is specified via CLI, could we just take that as the true value - so not overridden by user defined value?

@objectiser objectiser merged commit c24fa21 into jaegertracing:master Nov 19, 2020
@jpkrohling
Copy link
Contributor

About the decrease in the code coverage: looks like the error conditions aren't being tested:

image

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.

Document url not updated on jaeger when operator update it.
3 participants