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

chore: parametrize services Dockerfile path #1600

Merged
merged 8 commits into from
Jun 19, 2024

Conversation

rogercoll
Copy link
Contributor

@rogercoll rogercoll commented Jun 11, 2024

Changes

Define services paths and Dockerfiles using and environment variable so it can be easily overridden. With the added variables, a vendor can easily point to another Dockerfile using the .env.override file:

PAYMENT_SERVICE_DOCKERFILE=./src/paymentservice/Dockerfile.custom

That would help to make changes additive, thus minimizing merge conflicts.
In addition, the demo can also benefit from it and add different Dockerfile for the services and reflect different Otel deployment strategies.

The PR also aligns the emailservice Dockerfile with the other's services strategy by expecting to be built from the root's repository path.

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

* [ ] CHANGELOG.md updated to document new feature additions
* [ ] Appropriate documentation updates in the docs
* [ ] Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

@github-actions github-actions bot added the helm-update-required Requires an update to the Helm chart when released label Jun 11, 2024
@julianocosta89
Copy link
Member

Hey @rogercoll 👋🏽

This is interesting, and I like where this is going.
One note thought, would you be able to change the env vars to contain the whole path?

That would avoid the concatenation in the compose file and it would help when things do not go as expected.
WDYT?

@rogercoll
Copy link
Contributor Author

One note thought, would you be able to change the env vars to contain the whole path?

Definitely, it will be even more helpful for vendors that want to completely move a service to a different directory. Something like 8a40cd0 ?

PS I will bring this PR/idea to today's SIG meeting

@rogercoll rogercoll marked this pull request as ready for review June 13, 2024 10:34
@rogercoll rogercoll requested a review from a team June 13, 2024 10:34
@julianocosta89
Copy link
Member

@rogercoll, with 2 env vars to do the solution you mention we would need to update 2 places:

PAYMENT_SERVICE_DOCKERFILE=./src/paymentservice/Dockerfile.custom

My initial thought was having 1 env var containing the full path, do you see other use cases where having 2 would make more sense?

@rogercoll
Copy link
Contributor Author

do you see other use cases where having 2 would make more sense?

Just the ability to point the whole service to another directory, for example, let's say we do a copy of the payment service into ./src/payment-custom. Although the repository is not fully prepared for this feature (we would need to use the env inside the services Dockerfiles), it would allow overriding the whole service's path, not only the Dockerfile.

we would need to update 2 places

That would not be mandatory, as only the *_DOCKERFILE environment variable is used in the docker-compose files. In your .env.override file, you will just need to change the *_DOCKERFILE variable. The following solutions would work:

PAYMENT_SERVICE_DOCKERFILE=./src/paymentservice/Dockerfile.custom

or

PAYMENT_SERVICE_DOCKERFILE=${PAYMENT_SERVICE_PATH}/Dockerfile.custom

I don't have a strong opinion on the *_PATH environment variables, if you think it could complicate the usability we could go with a single environment variable pointing to the Dockerfile path for now, and revisited if/when needed. @julianocosta89

@puckpuck
Copy link
Contributor

I would like to use a single variable for each service containing the path and name of the Dockerfile. If someone needs to update this, they only need to update a single spot to reflect either a new folder or a new Dockerfile name.

@julianocosta89
Copy link
Member

I would like to use a single variable for each service containing the path and name of the Dockerfile. If someone needs to update this, they only need to update a single spot to reflect either a new folder or a new Dockerfile name.

me too!

@rogercoll
Copy link
Contributor Author

@julianocosta89 @puckpuck 👍 We can keep it simple for the moment and just use the Dockerfile variable.

Path environment variable removed in d328586

Copy link
Contributor

@puckpuck puckpuck left a comment

Choose a reason for hiding this comment

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

Thanks for this.

@julianocosta89 julianocosta89 merged commit 8a211c0 into open-telemetry:main Jun 19, 2024
28 checks passed
@rogercoll rogercoll deleted the parametrize_dockerfile branch June 19, 2024 12:23
AlexPSplunk pushed a commit to splunk/edu-opentelemetry-demo that referenced this pull request Jul 10, 2024
* chore: parametrize services Dockerfile path

* chore: add ad service relative path

* chore: add services path as environment variable

* ci: fix emailservice context build

* remove unused *_PATH environment variable

---------

Co-authored-by: Juliano Costa <julianocosta89@outlook.com>
ahealy-newr pushed a commit to ahealy-newr/opentelemetry-demo-ahealy that referenced this pull request Jul 24, 2024
* chore: parametrize services Dockerfile path

* chore: add ad service relative path

* chore: add services path as environment variable

* ci: fix emailservice context build

* remove unused *_PATH environment variable

---------

Co-authored-by: Juliano Costa <julianocosta89@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm-update-required Requires an update to the Helm chart when released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants