-
Notifications
You must be signed in to change notification settings - Fork 47
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
Remove topic creation from monasca/kafka #138
Conversation
Moved kafka topic creation to external image. That allows reusing the image from different locations. Closes #134
kafka-init/README.md
Outdated
|
||
|
||
[1]: http://semver.org/ | ||
[2]: https://github.com/monasca/monasca-docker/kafka |
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.
This will give 404
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.
Done, thx
kafka-init/README.md
Outdated
| `KAFKA_TIMEOUT` | `60` | How long to wait for Kafka to become available | | ||
| `KAFKA_CREATE_TOPICS` | `unset` | Topics to create on startup, see below | | ||
| `KAFKA_TOPIC_CONFIG` | `unset` | Default config args for created topics | | ||
| `KAFKA_CREATE_TOPICS_SCRIPT` | `KAFKA_CREATE_TOPICS_SCRIPT` | Path to script that creates topics | |
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 default be /kafka/bin/kafka-topics.sh
?
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.
Thx, done.
kafka-init/README.md
Outdated
``` | ||
|
||
As an example, this is a valid `KAFKA_CREATE_TOPICS` string for [Monasca][8] | ||
installations as used in the [docker-compose][4] environment: |
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.
Looks like these links are broken
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.
Fixed, also removed unused refs from kafka/README.md
This seems to work fine in docker-compose. We'll need to bump the version in Also, we'll need to coordinate things a bit with monasca-helm - can't bump the kafka version there until we have a kafka-init chart! |
kafka-init/Dockerfile
Outdated
ARG KAFKA_VERSION | ||
ARG SCALA_VERSION | ||
|
||
FROM monasca/kafka:${KAFKA_VERSION:-"0.9.0.1"}-${SCALA_VERSION:-"2.11"} |
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'm not sure if it's a big issue or not, but the pr-bot will not be able to parse this FROM
tag (and I'm not sure there's a reasonable way to parse it given the the bot's resource model). So this image won't get automatic updates from the base kafka image. We can either live with that (really not too bad) or use a plain FROM monasca/kafka:0.9.0.1-2.11
.
BTW, any reason to use ${KAFKA_VERSION:-"0.9.0.1"}
rather than just ARG KAFKA_VERSION=0.9.0.1
? Does docker not evaluate them properly in FROM
or anything like that?
(it might also be good to depend on the explicit container version somehow, e.g. monasca/kafka:${KAFKA_VERSION}-${SCALA_VERSION}-1.0.2
)
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.
Replaced that with single ARG. Basically there's no point in having multiple variables as we inherit from the monasca/kafka
image here. Should be much simpler now and easier to parse.
kafka-init/build.yml
Outdated
variants: | ||
- tag: 0.0.1 | ||
aliases: | ||
- ':latest' |
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.
super nit pick, this line is over-indented
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.
Done
Upgraded minor version to |
Changed buliding of kafka-init image to support just single argument as marker of parent monasca/kafka image. Bumped version of monasca/kafka to ```0.9.0.1-2.11-1.1.0``` from ```0.9.0.1-2.11-1.0.2```.
;-(...seems like it does not build because it cannot find locally build (or kafka-init build happens prior to tagging new monasca/kafka). Are there any changes required in the ci.py or dbuild to pick up such nits and organize the build or should I split the PR into 2 ? |
Tested this in my local env and it works. Topics were created in Kafka and I was able to list them. |
kafka/build.yml
Outdated
- :latest | ||
args: | ||
KAFKA_VERSION: "0.9.0.1" | ||
SCALA_VERSION: "2.11" | ||
- tag: 0.9.0.1-2.11-1.0.2 |
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.
This tag should actually be removed or it will overwrite the existing image (needed in helm for now) with the same content as the new version.
re: helm, that's no problem, we'll get around to adding it eventually - this has been on our to-do list for a while now, so you've already saved us a good bit of work 😄 As for the failure, it looks like you're right, kafka-init is being built before the new kafka image. Unfortunately the only way to guarantee they'll build in the correct order is to split the PR. It would be nice to support some dependency chains but that would require changes to both the CI script and dbuild, since dbuild doesn't guarantee build order (as it may build in parallel). I filed monasca/dbuild#2 to track improvements to this, but I likely won't be able to prioritize it for a while. |
Ok, I've splitted the PR, but unfortunately I expected only |
The variant for |
kafka/build.yml
Outdated
- tag: 0.9.0.1-2.11-1.1.0 | ||
args: | ||
KAFKA_VERSION: "0.9.0.1" | ||
SCALA_VERSION: "2.11" | ||
- tag: 0.9.0.1-2.11-1.0.2 |
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.
This -1.0.2
variant should be removed from build.yml entirely
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.
Ok, I've removed the variant.
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.
@timothyb89 is it like an "official approach"- we should drop older entries in build.yml
to disallow overriding older images with changes meant for newer ones ?
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.
@kornicameister I think so, yes. It's less of an issue for images that use date+time stamped tags since they'll always end up unique, but we definitely want to avoid introducing breaking changes by overwriting a tag that looks like it should be unique (like semver)
Remove variant ```-1.0.2``` to disallow overriding it with changes meant for ```1.1.0``` variant.
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.
Alright, looks good, let's try this thing!
FYI master will be broken until #153 merges. |
Kafka init
Moved kafka topic creation to external image. That allows reusing the image from different locations.
Partially_closes #134