-
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, Take 2) #2946
Conversation
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 seems to have no Jira tag, though the changes themselves look OK. 😦
Sorry -- I got distracted; I've added it. |
While that's nice, the Jira integration doesn't care about the PR description: only the git log messages, the PR title, or the branch name. |
Hmmm...I thought I was following what you did in #2941, but I failed to note that you'd included the tag in the first commit's message. If/when I rebase next, I'll amend it. (In the meantime, I've added the link to Jira manually.) |
Right, the PR description by default is the initial git log message; I usually just squash the newlines to let GitHub wrap it, but otherwise leave it alone. Adding the link manually to Jira isn't "satisfying" as that doesn't go into the Development pane, and doesn't trigger the automated state change. (Right now we're only automating In Progress when a PR is linked, but there are others we could use, and they're driven off the Development not the general links.) |
When I run
Is that a missing dependency somewhere? |
Yeah, I see that too (minus the fancy colors...):
The "vulnerabilities" statement bothers me, but I've been willfully ignoring it, hoping that @shubham-html-css-js or @MVarshini will tell be that it's "normal" and "nothing to be worried about". As for the Tox
See if that works for you. If so, then there's something missing from your system. (Maybe your Tox is old?) |
Yes, |
I prefer ignoring these warnings.And I also think this is the best practice to do so.Behind the scenes,The NPM registry runs a security audit on NPM packages. With the release of NPM v6, this command is run automatically when you execute an npm install on your project. Now should we be worried about these warnings? I prefer not to.In the world of reusable packages, a package can depend on other packages, creating a web of dependencies and the NPM audit command is checking all those dependencies, including those someone else has set up.These are dependencies someone else has added to their packages.If we were to change the dependencies someone else has taken, we cannot expect nothing adverse will happen.Maybe it will work just fine, but it does not sounds like a trivial change. The best way to fix this would be go the repo of the package, fork it ,modify the dependency and replace it with the fixed version, submit the PR and when the PR gets merged and then use patched version, but I guess that is out of scope for now. |
Also, run Python linters outside of Tox PBENCH-694
(Hopefully, these will be merged from other PRs.)
@dbutenhof, I've rebased the branch and added the Jira tag to the first commit.
😆 That's my preference, as well, @shubham-html-css-js; but I think we have a requirement to avoid security hazards and other critical bugs, and I'm concerned that ignoring the |
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.
Looks Good, also it ran smoothly on my system. jenkins/run ./build.sh
worked for me.
PBENCH-694
This PR is the second try at 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 of those, running lint and style checks as well as unit tests for the Agent, Server, and Dashboard. (This PR is a split of #2935, deferring the building of RPMs and subsequent stages to a subsequent PR.)
For convenience, this PR consists of a series of commits, but they should be squashed into a single commit when it is 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 this 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
.jenkins/run
script to 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).