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

PR CI should cover container image build [RHELDST-2973] #29

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

dichn
Copy link
Collaborator

@dichn dichn commented Sep 11, 2020

Add image build by using docker in travis-ci.

@dichn
Copy link
Collaborator Author

dichn commented Sep 11, 2020

Can capture the build failure in travis-ci job log.

In --require-hashes mode, all requirements must have their versions pinned with ==. These do not:
    idna-ssl>=1.0; python_version < "3.7" from https://files.pythonhosted.org/packages/46/03/07c4894aae38b0de52b52586b24bf189bb83e4ddabfe2e2c8f2419eec6f4/idna-ssl-1.1.0.tar.gz#sha256=a933e3bb13da54383f9e8f35dc4f9cb9eb9b3b78c6b36f311254d6d0d92c6c7c (from aiohttp==3.6.2->-r requirements.txt (line 15))
    dataclasses>=0.6; python_version < "3.7" from https://files.pythonhosted.org/packages/e1/d2/6f02df2616fd4016075f60157c7a0452b38d8f7938ae94343911e0fb0b09/dataclasses-0.7-py3-none-any.whl#sha256=3459118f7ede7c8bea0fe795bff7c6c2ce287d01dd226202f7c9ebc0610a7836 (from pydantic==1.6.1->-r requirements.txt (line 119))
The command '/bin/sh -c microdnf -y install shadow-utils     && microdnf -y install python3 python3-devel gcc make     && cd /usr/local/src/exodus-gw     && pip3 install --require-hashes -r requirements.txt     && pip3 install --no-deps .     && microdnf clean all && rm -rf /var/cache/yum && rm -rf /usr/local/src/exodus-gw' returned a non-zero code: 1
The command "docker build . -f openshift/containers/exodus-gw/Dockerfile" failed and exited with 1 during .

@crungehottman
Copy link
Member

@dichen16 - #25 has been merged, which should fix the Travis CI failure

@crungehottman
Copy link
Member

Am I correct to assume that intent of this PR is to validate that the image can be built successfully, so we can more easily detect whether a change will break an image build?

@dichn
Copy link
Collaborator Author

dichn commented Sep 11, 2020

@crungehottman yes, "validate that the image can be built successfully" exactly.

Copy link
Member

@rohanpm rohanpm left a comment

Choose a reason for hiding this comment

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

I don't think this is implemented in the right way - if you have a look at the Travis logs, you can see that it builds the container image at the beginning of each of the three jobs in the test matrix, although as far as the build is concerned, there is no difference in environment between them.

It's at least wasteful, and also seems arbitrary to handle this one kind of test differently in this way. Before the change, we had this:

  • one job running python tests & collecting coverage
  • one job building docs
  • one job applying static checkers (e.g. pylint)

If we want to add another kind of test, should that just be adding one more job to the above list? It seems more logical than adding an unrelated step at the beginning of all the existing jobs.

@dichn dichn marked this pull request as draft September 14, 2020 02:08
@dichn dichn marked this pull request as ready for review September 14, 2020 02:46
@dichn
Copy link
Collaborator Author

dichn commented Sep 14, 2020

@rohanpm separated job for build-docker-image added.

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
Add image build by using docker in travis-ci.
Copy link
Contributor

@lmilbaum lmilbaum left a comment

Choose a reason for hiding this comment

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

Building the image and pushing it to right registry should be handled in a separate task before the CI tests are executed.

@dichn
Copy link
Collaborator Author

dichn commented Sep 14, 2020

@lioramilbaum This PR introduces another image-building process besides the Quay.io Build Triggers.

  1. Travis-CI build the image for validating that the image can be built successfully. This check happens at every PRs.
  2. Quay.io build the image and store it in registry after a PR is merged(unit tests are all passed, and reviews are done).

So building-image is happened before CI tests. Not sure why pushing to registry should happened before CI tests as well. Then untested, risky image can exist in our registry, right?
PS. The CI tests above are unit tests.

@lmilbaum
Copy link
Contributor

The long term plan is to enable integration tests in CI. Unit tests are not sufficient validation.
Will approve the PR as it is.

@dichn
Copy link
Collaborator Author

dichn commented Sep 14, 2020

Yes, upstream CI is not enough, downstream CI for integration test running in openshift is necessary.

@rohanpm rohanpm merged commit f3968ed into release-engineering:master Sep 14, 2020
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.

None yet

5 participants