Refactor Jenkins build tests #300
Conversation
There was a problem hiding this comment.
+@BetsyMcPhail for feature review
Reviewable status: all discussions resolved, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @nicolecheetham)
-- commits line 2 at r1:
BTW As this PR is actually making a functional change by removing some tests from Jenkins, it would be good to explicitly mention that here. That would make it clear that they aren't being removed by accident.
Related, the "Continuous Integration" section of README.md needs to be updated to account for these removals.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)
Previously, BetsyMcPhail (Betsy McPhail) wrote…
BTW As this PR is actually making a functional change by removing some tests from Jenkins, it would be good to explicitly mention that here. That would make it clear that they aren't being removed by accident.
Related, the "Continuous Integration" section of README.md needs to be updated to account for these removals.
Thank you for pointing this out. I have updated the description of the PR to mention the functional change as well as the README.md
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! all discussions resolved, platform LGTM from [betsymcphail] (waiting on @nicolecheetham)
There was a problem hiding this comment.
+@jwnimmer-tri for platform review
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, platform LGTM from [betsymcphail, jwnimmer-tri] (waiting on @nicolecheetham)
-- commits line 2 at r2:
When this merges, we need a much better summary -- either with a force-push to adjust the commit message, or edit it during the squash-and-merge using the web interface.
The verb "refactor" indicates a cleanup with no change in functionality, but here we have two important changes in functionality:
(1) Remove three build flavors from Jenkins because they are redundant with GHA:
- drake_bazel_download
- drake_catkin_installed
- drake_cmake_installed
(2) In the remaining two build flavors in Jenkins, run each on a separate machine, so their setup script and/or results do not contaminate each other.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, platform LGTM from [betsymcphail, jwnimmer-tri] (waiting on @jwnimmer-tri)
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
When this merges, we need a much better summary -- either with a force-push to adjust the commit message, or edit it during the squash-and-merge using the web interface.
The verb "refactor" indicates a cleanup with no change in functionality, but here we have two important changes in functionality:
(1) Remove three build flavors from Jenkins because they are redundant with GHA:
- drake_bazel_download
- drake_catkin_installed
- drake_cmake_installed
(2) In the remaining two build flavors in Jenkins, run each on a separate machine, so their setup script and/or results do not contaminate each other.
I don't believe 2) was implemented in this PR... I'm not sure if that was intentional or not? Nicole, I'm happy to take the Jenkins refactor (splitting into two jobs) as a follow-up to this PR if you would like.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, platform LGTM from [betsymcphail, jwnimmer-tri] (waiting on @BetsyMcPhail)
Previously, BetsyMcPhail (Betsy McPhail) wrote…
I don't believe 2) was implemented in this PR... I'm not sure if that was intentional or not? Nicole, I'm happy to take the Jenkins refactor (splitting into two jobs) as a follow-up to this PR if you would like.
Ah, you're right. I misread the groovy.
In the case it's just (1) as the only functional change.
I would also be OK to mention something like "refactor code to remove layers of confusing indirection" in the commit message, if you like.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, platform LGTM from [betsymcphail, jwnimmer-tri] (waiting on @BetsyMcPhail and @jwnimmer-tri)
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Ah, you're right. I misread the groovy.
In the case it's just (1) as the only functional change.
I would also be OK to mention something like "refactor code to remove layers of confusing indirection" in the commit message, if you like.
Sure. Splitting the two jobs to run on a separate machine can be a follow-up to this PR once this one is merged
And I'll see if I can include both the build refactoring and the removal of the redundant build flavors into a commit message. If not, I can more closely use your suggestion.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 1 unresolved discussion, platform LGTM from [betsymcphail, jwnimmer-tri] (waiting on @BetsyMcPhail)
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, platform LGTM from [betsymcphail, jwnimmer-tri] (waiting on @nicolecheetham)
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, platform LGTM from [betsymcphail, jwnimmer-tri] (waiting on @jwnimmer-tri and @nicolecheetham)
Previously, nicolecheetham (Nicole C.) wrote…
Sure. Splitting the two jobs to run on a separate machine can be a follow-up to this PR once this one is merged
And I'll see if I can include both the build refactoring and the removal of the redundant build flavors into a commit message. If not, I can more closely use your suggestion.
Follow-up issue: #303
nicolecheetham
force-pushed
the
jenkins_build_refactor
branch
from
8fb2c4b
to
58f439c
Compare
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
Cleans up Jenkins build tests workflow making it more clear and concise
Additionally, removes build tests in the Jenkins that already exist as GHA build tests
Issue: #61
This change is