Skip to content

Conversation

rwaffen
Copy link
Member

@rwaffen rwaffen commented Jan 24, 2025

No description provided.

@rwaffen rwaffen requested a review from a team January 24, 2025 10:54
@rwaffen rwaffen enabled auto-merge January 24, 2025 10:58
@rwaffen rwaffen merged commit a646c1a into main Jan 24, 2025
8 checks passed
@rwaffen rwaffen deleted the update_containerfile branch January 24, 2025 10:59
Comment on lines +81 to +85
ADD https://apt.overlookinfratech.com/openvox${OPENVOX_RELEASE}-release-ubuntu${UBUNTU_VERSION}.deb /
RUN apt-get update && \
apt-get install -y ca-certificates && \
dpkg -i /openvox${OPENVOX_RELEASE}-release-ubuntu${UBUNTU_VERSION}.deb && \
rm /openvox${OPENVOX_RELEASE}-release-ubuntu${UBUNTU_VERSION}.deb
Copy link
Contributor

Choose a reason for hiding this comment

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

If you have this in 2 commands, won't this end up in a layer that is still shipped to the end user?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. this was more like a test if i can somehow cache this layer in build time... but it does not.

Comment on lines +93 to +95
apt-get install -y openvox-agent=${OPENVOXAGENT_VERSION}-1+ubuntu${UBUNTU_VERSION} && \
apt-get install -y openvox-server=${OPENVOXSERVER_VERSION}-1+ubuntu${UBUNTU_VERSION} && \
apt-get install -y openvoxdb-termini=${OPENVOXDB_VERSION}-1+ubuntu${UBUNTU_VERSION} && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to specify the exact package name like this? By hardcoding -1 you're making sure you can't use releases to patch small things. Also, probably better to run it in a single apt-get install command. The server can pull in a different agent version and might up-/downgrade it this way. I suspect that with a single command it'll refuse to run instead. You can probably even combine it with $PACKAGES

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't know how to better pin the version. i tried package=version but this didnt work until i added the rest :(.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some experimentation tells me apt install openvox-agent=8.11.0-* works for me so I think this should work:

Suggested change
apt-get install -y openvox-agent=${OPENVOXAGENT_VERSION}-1+ubuntu${UBUNTU_VERSION} && \
apt-get install -y openvox-server=${OPENVOXSERVER_VERSION}-1+ubuntu${UBUNTU_VERSION} && \
apt-get install -y openvoxdb-termini=${OPENVOXDB_VERSION}-1+ubuntu${UBUNTU_VERSION} && \
apt-get install -y openvox-agent=${OPENVOXAGENT_VERSION}-* openvox-server=${OPENVOXSERVER_VERSION}-* openvoxdb-termini=${OPENVOXDB_VERSION}-* && \

ADD https://apt.overlookinfratech.com/openvox${OPENVOX_RELEASE}-release-ubuntu${UBUNTU_VERSION}.deb /
RUN apt-get update && \
apt-get install -y ca-certificates && \
dpkg -i /openvox${OPENVOX_RELEASE}-release-ubuntu${UBUNTU_VERSION}.deb && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Apt can install local files just fine

Copy link
Member Author

Choose a reason for hiding this comment

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

oh okay didnt know... will try that. is it like in yum?

Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of, I'd say yum does it a little better because you can also point it to http(s) URLs. With apt you need to get it somehow (like wget) and the use apt install /path/to/file.deb. You can't use a relative path so you must use ./file.deb if it's in the current directory.

@ekohl
Copy link
Contributor

ekohl commented Jan 24, 2025

And I missed that you already merged it when I was reviewing.

@rwaffen
Copy link
Member Author

rwaffen commented Jan 24, 2025

sry, auto merge was on...

@rwaffen rwaffen added the enhancement New feature or request label Jan 24, 2025
@rwaffen rwaffen moved this to Done in Container Todos May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants