-
Notifications
You must be signed in to change notification settings - Fork 108
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
Enhancements to the CI pipeline, part 1 #2935
Conversation
e8ff33f
to
5d7f8fd
Compare
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.
Cool!
1d2bd19
to
b8ac1d6
Compare
27e0d0b
to
57d006f
Compare
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.
OK, it seems to be working fairly well, so I think it's ready for review! Thanks!! (And, see you Monday....)
# victory now. (Building a container inside a container requires a | ||
# specially-configured and/or -invoked container, and we don't have that | ||
# arranged, yet.) | ||
exit 0 |
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.
Why do we need everything performed in one container?
Why can't the pipeline be broken up into steps, some containerized, some not?
We can also have Jenkins start a particular container environment for us instead of running one monolithic one.
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.
I believe that we should have everything performed by one command. Now whether that command runs a single script which invokes a series of containers (or whatnot), or whether than command invokes a single container which executes a series of actions is certainly open for discussion. But, what I think we want to avoid is putting lots of stages in the Pipeline.gy
file, because then it won't be feasible for developers to run "the CI tests" locally.
The advantage of running it all in a single container (or, perhaps, running each of "The Things" in containers) is that we (collectively and individually) don't have to be concerned about what the respective environments look like. And, if we could run it all in one container, then that would greatly reduce the amount of administration required to run the test pipeline.
But, no, it doesn't have to be that way. It just seemed, at the outset, like a good idea. I would be happy to hear your thoughts on the matter.
# Test for code style and lint | ||
black --check . | ||
flake8 . | ||
( cd dashboard && npx eslint "src/**" --max-warnings 0 ) |
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.
This change means we don't get these checks in local tox runs. Isn't that bad?
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.
Why would that be bad?
I mentioned above the desire to have the pipeline run by a single command, but that command doesn't necessarily have to be tox
. We have a series of steps -- linters, unit tests, rpm builds, container builds, and perhaps functional tests -- how many of those would you expect to run under tox
? Would you expect that Dashboard tests, which have nothing to do with Python be run under tox
? (Is it important to have the Python linters run under a particular version of Python, i.e. which Tox provides?)
I'm suggesting that we "switch" the one command from tox
to ./build.sh
. Would that fit your requirements?
`#` \ | ||
python3-jinja2-cli \ | ||
rpmlint \ | ||
http://hdn.corp.redhat.com/rhel8-csb/RPMS/noarch/redhat-internal-cert-install-0.1-25.el7.noarch.rpm \ |
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.
Why is this in the public repo?
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.
Well...I guess that's a good question.
The logic goes like, "The CI container needs those certs, so they are installed as part of the container recipe, which we currently store in the public repo...." But, yes, that's an internal network name, reachable only through the tunnel.
So, how should we proceed?
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.
Why does the CI container need those certs?
|
||
# Run unit tests | ||
tox # Agent and Server unit tests and legacy tests | ||
( cd dashboard && CI=true npm test ) # Dashboard unit tests |
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.
Why not add this to tox?
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.
Why should it be? See my previous.
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.
Let's move the coverage fix to a separate PR.
(Hopefully, these will be merged from other PRs.)
I've converted this PR back to draft mode while I attack the following issues:
@portante and @dbutenhof, please resolve any of your outstanding conversations which are no longer an issue for this PR (e.g., Dave, this PR no longer deals with Cobertura, so the coverage report issues will be addressed in a different PR). In the meantime, I think I'm going to split off the building of the RPMs to a subsequent PR and limit this PR to just lint, unit tests, and maybe source-RPMs. The additional issues I mentioned at the start here will show up in a subsequent PR. |
This PR is the first installment in a series of enhancements to the Pbench CI pipeline. The goal is to have the CI perform lint and style checks, run unit tests, build RPMs, build containers, and run functional tests for each of the Agent, Server, and Dashboard. This PR provides the first several of those, up through building RPMs for the Agent and Server (the Dashboard is expected to be deployed using a different mechanism). It also enables the Cobertura report to find and display the source lines!
For convenience, this PR consists of a series of commits, but they should be squashed into a single commit when it is merged (NOTE: this PR is now based on #2938 and #2939, so their respective commits will appear in this PR until they are merged):
Highlights of the changes:
build.sh
which executes the steps outlined above. The CI executes this script in the CI container via thejenkins/run
command. The intention is that developers should be able to run the script as well, either natively or via ajenkins/run
command in their own environment. The script does the following:tox
directly to running the newbuild.sh
scriptbuild.sh
, and add to the dependences for the linters in the CI container image definition.jenkins/run
script to add a mapping for the COPR config file from the Jenkins secret file (or the user's file) to the conventional file inside the container. Also, add a volume for the${HOME}/.config
directory (so thenpm
install inside the container can write it), and reorder the volume options in the container invocation from top to bottom (i.e.,${HOME}
first, then${HOME}/.config
, and so on).copr-ci
target.