Skip to content
This repository has been archived by the owner on May 18, 2020. It is now read-only.

Jaeger Version++ && Version documentation #89

Merged
merged 5 commits into from
Jun 19, 2018

Conversation

Aergonus
Copy link
Contributor

Similar to #70, addresses #88

Signed-off-by: Victor Lei KLEI22@bloomberg.net

@yurishkuro
Copy link
Member

using your real name (sorry, no pseudonyms or anonymous contributions.)

@Aergonus
Copy link
Contributor Author

Heh... I see. DCO pulls from git config.
dcoapp/app#43

@Aergonus
Copy link
Contributor Author

Aergonus commented May 22, 2018

It seems like your CI jobs are a bit broken.
The only change in the commit from before aka #254 is the commit sign-off, yet it failed CI on the next run #255.

Similar to jaegertracing#70, addresses jaegertracing#88

Signed-off-by: Victor Lei <KLEI22@bloomberg.net>
@yurishkuro
Copy link
Member

I see. DCO pulls from git config. dcoapp/app#43

I don't think DCO has access to your git config. I am not sure how in that ticket the Author is different from the sign-off, wouldn't they both come from git config?

Personally I recommend using your full name in the GitHub profile, I think it takes care of the issue. It's not like your full name is a secret if you're putting it in the commit signature anyway.

README.md Outdated
@@ -147,6 +147,10 @@ kubectl run jaeger-spark-dependencies --schedule="55 23 * * *" --env="STORAGE=ca

If you want to run the job only once and immediately then remove scheduled flag.

## Docker Versions
The jaeger project automatically creates new docker images with versions that mirror the release number. Feel free to pick the latest release.
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/jaeger/Jaeger

Copy link
Collaborator

Choose a reason for hiding this comment

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

s/docker/Docker

README.md Outdated
@@ -147,6 +147,10 @@ kubectl run jaeger-spark-dependencies --schedule="55 23 * * *" --env="STORAGE=ca

If you want to run the job only once and immediately then remove scheduled flag.

## Docker Versions
The jaeger project automatically creates new docker images with versions that mirror the release number. Feel free to pick the latest release.
> A general k8 tip; it's recommended that you do not use `:latest` in production but rather pin the latest version.
Copy link
Collaborator

Choose a reason for hiding this comment

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

k8? Did you mean Kubernetes (k8s)? I think this tip is more general than that, so:

Docker pro-tip: do not use :latest in production, but rather, pin it to the latest release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct the tags are parsed by docker, but seeing as this is a kubernetes repo and most people are deploying jaeger with kubernetes, tagging this as a kubernetes/deployment related tip would be less confusing.

@Aergonus Aergonus force-pushed the version++ branch 2 times, most recently from 8d3e4ed to 4f27723 Compare June 16, 2018 21:15
@Aergonus Aergonus mentioned this pull request Jun 16, 2018
@Aergonus
Copy link
Contributor Author

ping!

Address PR comments and run CI tests again

Signed-off-by: Victor Lei <klei22@bloomberg.net>
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

@Spellchaser thanks for the PR!

I don't think that readme section about tags is necessary as we use pinned versions.

@@ -12,6 +12,10 @@
# the License.
#

#
# Check https://github.com/jaegertracing/jaeger/releases for the latest version to use
Copy link
Member

Choose a reason for hiding this comment

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

can you please remove this?

Copy link
Member

Choose a reason for hiding this comment

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

This should rather link to dockerhub but I think everybody knows what registry it is using and where to find more info

@@ -33,7 +37,7 @@ items:
jaeger-infra: collector-pod
spec:
containers:
- image: jaegertracing/jaeger-collector:1.2
- image: jaegertracing/jaeger-collector:1.4.1
Copy link
Member

Choose a reason for hiding this comment

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

The newest is 1.5.0

@@ -120,7 +120,8 @@ items:
spec:
containers:
- name: jaeger-cassandra-schema
image: jaegertracing/jaeger-cassandra-schema:1.2
image: jaegertracing/jaeger-cassandra-schema:1.4.1
# Check https://github.com/jaegertracing/jaeger/releases for the latest version
Copy link
Member

Choose a reason for hiding this comment

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

The comment is not needed

@Aergonus
Copy link
Contributor Author

Right so the pinned versions here are seldom updated. For any new user coming to the project, it's better to at least have a comment warning them that this may not be the latest version.

I spent more time than I'd like to admit on trying to get base-path to work when it was changed to prefix.

Signed-off-by: Victor Lei <klei22@bloomberg.net>
@pavolloffay
Copy link
Member

Right so the pinned versions here are seldom updated. For any new user coming to the project, it's better to at least have a comment warning them that this may not be the latest version.

I don't see such a comment in the readme. It would be better to add a sentence to Development setup about using latest and somewhere to production setup about using pinned version.

Signed-off-by: Victor Lei <klei22@bloomberg.net>
@Aergonus
Copy link
Contributor Author

The process for determining what version is useable for production seems to be the latest release for jaeger. Are you relying on the CI jobs to test the new versions for production?

@pavolloffay
Copy link
Member

The pinned version is used to not accidentally break people on new releases. I wanted also tag this repo to version templates too as there can be changes too.

@Aergonus
Copy link
Contributor Author

Uhhh lemme change the README

@pavolloffay
Copy link
Member

np I will wait

Don't tell people "Feel free to use the latest version"

Signed-off-by: Victor Lei <klei22@bloomberg.net>
@Aergonus
Copy link
Contributor Author

I think that's valid english :)
I figured that you didn't want that line in the docker images part

@pavolloffay pavolloffay merged commit e19d39b into jaegertracing:master Jun 19, 2018
@pavolloffay
Copy link
Member

@Spellchaser thanks

@pavolloffay pavolloffay mentioned this pull request Jun 19, 2018
@Aergonus Aergonus deleted the version++ branch June 26, 2018 20:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants