Skip to content
This repository has been archived by the owner on May 24, 2023. It is now read-only.

integration.Dockerfile: image for integration tests #93

Closed
wants to merge 1 commit into from
Closed

integration.Dockerfile: image for integration tests #93

wants to merge 1 commit into from

Conversation

estroz
Copy link

@estroz estroz commented Sep 19, 2019

Image containing all runtime and integration test dependencies. This should be built on each commit to master and pushed remotely, and ideally on every release. The idea is to use this as a base image for operator-courier's as well as OMPS' integration tests.

/cc @csomh @MartinBasti @shawn-hurley @gallettilance

@csomh csomh self-requested a review September 20, 2019 09:00
@@ -0,0 +1,35 @@
FROM fedora:29
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not fedora:30 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not fedora:latest? 😄

Based on my experience, it's OK to assume the risk of using :latest, as it is smaller than the risk of forgetting to upgrade to the newest release. And Fedora has a quite fast release cycle, which would mean a new change every 6 month.

Copy link
Contributor

@csomh csomh left a comment

Choose a reason for hiding this comment

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

See comments inline. Furthermore:

  • I would find it more logical to create this as tests/integration/Dockerfile
  • tests/integration/README.md should be updated to explain the usage of this file.
  • Who/what is going to be responsible to rebuild this and push it (where?) - would be nice to update the commit message with this information and maybe also mention it in the integration test README.
  • Please update the commit message with a general explanation why this change is made - this repo is configured to rebase and merge PRs, so there are not merge commits created. This means everything should be in the commit messages, as PR descriptions won't make it to the history.

@@ -0,0 +1,35 @@
FROM fedora:29
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not fedora:latest? 😄

Based on my experience, it's OK to assume the risk of using :latest, as it is smaller than the risk of forgetting to upgrade to the newest release. And Fedora has a quite fast release cycle, which would mean a new change every 6 month.

license="GPLv3"

# Test and build dependencies.
RUN dnf -y install \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
RUN dnf -y install \
RUN dnf -y install --setopt=tsflags=nodocs --setopt=install_weak_deps=false \

The above would make the resulting image smaller, by not installing documentation and weak dependencies.

&& dnf -y clean all \
&& rm -rf /tmp/*

RUN pip3 install tox coveralls
Copy link
Contributor

Choose a reason for hiding this comment

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

Both of these are available from the Fedora repos. Wouldn't it be better to install them from there?


WORKDIR /src
COPY . .
RUN pip3 install -e .
Copy link
Contributor

@csomh csomh Sep 20, 2019

Choose a reason for hiding this comment

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

Do you expect this to install only OMPS? I.e: all other dependencies should be satisfied by previous dnf installs?

If yes, then using --no-deps could make sure, that there are no additional dependencies pulled in from PyPI.

@estroz
Copy link
Author

estroz commented Sep 20, 2019

@csomh is there no CI process that automates image builds? I figured this image would be lumped in with those.

Side note: I figured out an easier way to create an OMPS image without requiring one be built by this repo. If you would still like this Dockerfile merged, I will address your comments. Otherwise I will close this PR.

@csomh
Copy link
Contributor

csomh commented Sep 23, 2019

@estroz quay.io/martinbasti/omps is built by Quay, but this needs a build trigger to be set up.

re: side note: I'll close this PR then :) Thanks!

@csomh csomh closed this Sep 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants