-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add container support for Ibeji #62
Add container support for Ibeji #62
Conversation
################################################################################ | ||
# Create a stage for building the application. | ||
|
||
ARG RUST_VERSION=1.72.1 |
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.
is there a way for this to be controlled by the rust-toolchain.toml file?
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.
Not in a simple way, there may be a way to copy the value from the rust-toolchain.toml file or install rust manually but I am not sure it is worth it at this point.
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.
If this goes in as is, then let's make it a high priority follow-up to address this. I have some ideas on how to do this. Perhaps we can have a 15 min brainstorming on the best way to do it and to make sure that the solution gets into all of our software components.
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.
Some minor comments. Please take a look.
################################################################################ | ||
# Create a stage for building the application. | ||
|
||
ARG RUST_VERSION=1.72.1 |
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.
If this goes in as is, then let's make it a high priority follow-up to address this. I have some ideas on how to do this. Perhaps we can have a 15 min brainstorming on the best way to do it and to make sure that the solution gets into all of our software components.
core/common/src/utils.rs
Outdated
#[cfg(feature = "containerize")] | ||
let uri = { | ||
// Container env variable names. | ||
const HOST_GATEWAY_ENV_VAR: &str = "HOST_GATEWAY"; |
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.
Perhaps these constants should be moved out of this function (perhaps to s shared constant code file) and made public?
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.
So the idea was to keep the constants with this function because it is only used in a container, and those values should be set by the .env files provided
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.
If they are only ever used once, then they probably don't even need to be constants.
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.
Two minor comments
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.
Approved.
core/common/src/utils.rs
Outdated
#[cfg(feature = "containerize")] | ||
let uri = { | ||
// Container env variable names. | ||
const HOST_GATEWAY_ENV_VAR: &str = "HOST_GATEWAY"; |
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.
If they are only ever used once, then they probably don't even need to be constants.
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.
Approve.
* Add dockerfiles and supporting configuration to Ibeji * removed uncessary build dependencies * ignore devskim warnings * resolve README comments * fixed typo * set IBEJI_HOME environment variable in dockerfile * Update get_uri to return what env variable the failure was for * fix devskim err * attempt 2 of fixing devskim * change var name as ignoring creates a fmt error * remove commented feature flag * changed const to variables * fix feature flag * Rearrange README
This PR adds containerization support for Docker and Podman. In addition:
Additionally: