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
Move Katello npm install to parallel step #1495
Conversation
withRVM(["bundle exec npm install"], ruby) | ||
} | ||
dir('katello') { | ||
sh 'npm test' |
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.
There was an 'npm install' step here, but I removed it as foreman's npm install will install for plugins. I didn't see any additional installation happening for this step in our current runs.
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.
Also, there was this condition:
when {
expression { fileExists('package.json') }
}
Is this still needed? It seems implied now. If it is still needed, I could use some help on how to add it for just a single action in a step.
d6673c0
to
f9822ae
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.
Can npm in the katello directory actually find the npm packages?
dir('foreman') { | ||
withRVM(["bundle exec npm install"], ruby) | ||
} | ||
dir('katello') { |
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 think you simply omit dir()
when you want katello
.
and combine react-ui and assets precompile. Both react tests and assets precompile need the installed npm packages, so we can combine them and install packages before running those tests. This will allow the long-running rails suite to not be blocked by an npm install.
f9822ae
to
c5dc609
Compare
@ekohl thanks, updated to not use
I'm not sure I'm understanding the question exactly, but |
We have moved the Jenkins job definitions over to https://github.com/theforeman/jenkins-jobs -- please re-open your PR against this new repository. |
and combine react-ui and assets precompile.
Both react tests and assets precompile need the installed npm packages, so we can combine them and install packages before running those tests. This will allow the long-running rails suite to not be blocked by an npm install.