-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
CI matrix change: debian+centos+sles #58988
CI matrix change: debian+centos+sles #58988
Conversation
Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer Issue DetailsChanges in CI matrix:
part of #57947
|
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.
Changes look good.
cc: @danmoseley
@@ -67,21 +65,18 @@ jobs: | |||
- RedHat.7.Amd64.Open | |||
- Ubuntu.1604.Amd64.Open | |||
- Ubuntu.1804.Amd64.Open | |||
- SLES.12.Amd64.Open | |||
- SLES.15.Amd64.Open |
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.
Should isFullMatrix==true
be a strict super set of isFullMatrix==false
?
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.
Yes, I've asked that in other PRs. That way we can track pass rate.
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.
@jkotas This is game changer. If I understand correctly, we are not able to run test for PR only as Rolling tests will run all PR tests, is that correct?
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.
Yes, I think it makes sense for PRs to run subset and for rolling to run everything (including the subset that PRs run).
I believe that it is the case today for the most part, there are just a few outliers.
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.
@ViktorHofer, If I the a Debian 10 © as an example: We want this update: Drop rolling (keep PR)
, was it meant to remove it only from isFullMatrix==true and leave it in isFullMatrix==false ?
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.
@aik-jahoda based on the discussion above, we don't want to drop rolling anywhere except for EOL distros. So "Drop rolling (keep PR)" should probably be ignored.
- (Debian.11.Amd64.Open)Ubuntu.1804.Amd64.Open@mcr.microsoft.com/dotnet-buildtools/prereqs:debian-11-helix-amd64-20210304164428-5a7c380 | ||
- (Mariner.1.0.Amd64.Open)ubuntu.1604.amd64.open@mcr.microsoft.com/dotnet-buildtools/prereqs:cbl-mariner-1.0-helix-20210528192219-92bf620 | ||
- ${{ if eq(parameters.jobParameters.isFullMatrix, false) }}: | ||
- (Centos.8.Amd64.Open)Ubuntu.1604.Amd64.Open@mcr.microsoft.com/dotnet-buildtools/prereqs:centos-8-helix-20201229003624-c1bf759 | ||
- RedHat.7.Amd64.Open | ||
- (Debian.10.Amd64.Open)Ubuntu.1804.Amd64.Open@mcr.microsoft.com/dotnet-buildtools/prereqs:debian-10-helix-amd64-20210304164434-56c6673 | ||
- (Debian.11.Amd64.Open)Ubuntu.1804.Amd64.Open@mcr.microsoft.com/dotnet-buildtools/prereqs:debian-11-helix-amd64-20210304164428-5a7c380 | ||
- Ubuntu.1604.Amd64.Open | ||
- Ubuntu.1804.Amd64.Open |
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 would drop Ubuntu16, SLES and Fedora from the isFullMatrix==false
runs.
RedHat+CentOS+Debian+Ubuntu should be more than good enough coverage for the various Linux flavors.
@jkotas , can you please review the table below to confirm we are on the same page?
|
@aik-jahoda Yes, this looks good. Thank you! |
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.
Thanks
Fails on RETHEL 8 which is out of scope of this change. |
Changes in CI matrix (updated):
part of #57947