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

Change operator.yaml to use master, to keep the readme uptodate with … #296

Merged
merged 1 commit into from
Mar 11, 2019
Merged

Conversation

objectiser
Copy link
Contributor

…latest version

Signed-off-by: Gary Brown gary@brownuk.com

…latest version

Signed-off-by: Gary Brown <gary@brownuk.com>
@jpkrohling
Copy link
Contributor

This change is Reviewable

@objectiser
Copy link
Contributor Author

To avoid problem reported in #293 .

@codecov
Copy link

codecov bot commented Mar 11, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@be83935). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #296   +/-   ##
=========================================
  Coverage          ?   88.52%           
=========================================
  Files             ?       70           
  Lines             ?     3110           
  Branches          ?        0           
=========================================
  Hits              ?     2753           
  Misses            ?      244           
  Partials          ?      113

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 be83935...a57284e. Read the comment docs.

@@ -16,7 +16,7 @@ spec:
serviceAccountName: jaeger-operator
containers:
- name: jaeger-operator
image: jaegertracing/jaeger-operator:1.10.0
image: jaegertracing/jaeger-operator:master
Copy link
Member

Choose a reason for hiding this comment

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

Is this a temporary fix?

If so add a note to release V1 issue that it has to be changed when releasing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if should be temporary - if we fix on a particular version, then the readme needs to reflect that version until the next stable version is released, rather than the current approach where we update the readme as changes are applied.

Something we should discuss when @jpkrohling is available.

Copy link
Member

Choose a reason for hiding this comment

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

I would expect (and I think most users too) that when you download https://github.com/jaegertracing/jaeger-operator/blob/v1.10.0/deploy/operator.yaml - (mind the tag version in the link) that it would deploy operator with the given version.

Copy link
Contributor

Choose a reason for hiding this comment

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

This change gets overridden every time we do a release:

sed "s~image: jaegertracing/jaeger-operator.*~image: ${BUILD_IMAGE}~gi" -i deploy/operator.yaml

This file is also used as the base for other operator-related files, like the test manifests and OLM CSV (IIRC).

To address the original problem this PR is fixing, I think we could mention in the documentation the version a change will take effect ("from version 1.11.0, the apiVersion should be ...")

@objectiser objectiser merged commit 26e5250 into jaegertracing:master Mar 11, 2019
@objectiser objectiser deleted the usemaster branch March 11, 2019 16:49
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