Skip to content

Conversation

@gadelkareem
Copy link
Contributor

Description

  • bump elasticsearch and kibana images version.
  • add datagen connector.
  • fix start.sh script to work on

Author Validation

Test start.sh on M1 and other OS

@chuck-confluent
Copy link
Contributor

Hey Waleed, thanks for the PR!

Could you help me understand which changes are related to mac m1 provisioning? It seems like there are a lot of changes here unrelated to the title of the PR (for example the addition of the "orders" datagen connector).

I notice the docker --no-recreate flags have been removed. I added those in the past to fix a race condition where docker would recreate the zookeeper container which would require manual intervention to fix. I haven't had any issues with the flag, but is that what's inhibiting m1? CP Demo isn't tested for m1.

@gadelkareem
Copy link
Contributor Author

gadelkareem commented Aug 30, 2022

Hey Chuck!

The Elasticsearch and Kibana containers were still amd64 which was causing data leak on M1 so the new ones are ARM64 ready. Also docker failed to provision while using the --no-recreate flag, I am not sure if it is related to a new version of docker or mac m1.

The datagen connector is not related indeed, but I thought it is nice to have in this repo.

@chuck-confluent
Copy link
Contributor

chuck-confluent commented Aug 30, 2022

Cool, I have no problem with the update to elastic. What error occurs with --no-recreate? Mac m1 should be able to do that. What version of docker for desktop and docker compose are you using? It might be an issue where docker-compose v1 is handling the flag differently than docker compose v2, so make sure to check your versions. Docker for desktop should, in theory, automatically alias docker-compose to docker compose.

We will not be adding the orders datagen to the at this time, so could you please restrict the PR to just the docker / ARM / m1 discussion?

@gadelkareem
Copy link
Contributor Author

I just tested it again now and did not see any errors while using --no-recreate. Maybe I was just using the old version of the file. I restored the modified file and kept only the ES and Kibana changes.

@chuck-confluent
Copy link
Contributor

Awesome! I will do a quick smoke test soon and merge. Thanks again!

@chuck-confluent chuck-confluent changed the base branch from master to 7.2.1-post August 30, 2022 19:00
@chuck-confluent
Copy link
Contributor

@gadelkareem FYI I need to rebase onto the default branch 7.2.1-post instead of master. We have our own idiosyncratic branching system. Sorry for the confusion

@chuck-confluent
Copy link
Contributor

chuck-confluent commented Aug 30, 2022

@gadelkareem actually could you rebase onto 7.2.1-post and push again? This PR is now trying to apply all the commits from master, and we don't want that. You might need to create a new PR

@gadelkareem
Copy link
Contributor Author

Sure, let me do that.

@gadelkareem
Copy link
Contributor Author

@chuck-confluent done!

@chuck-confluent chuck-confluent self-requested a review August 30, 2022 21:48
@chuck-confluent chuck-confluent merged commit c135a76 into confluentinc:7.2.1-post Aug 30, 2022
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.

4 participants