Skip to content

Conversation

@omajid
Copy link
Member

@omajid omajid commented Mar 30, 2022

This is to help catch OpenSSL 3.0 issues, like #67304

@ghost ghost added area-Infrastructure-libraries community-contribution Indicates that the PR has been added by a community member labels Mar 30, 2022
@ghost
Copy link

ghost commented Mar 30, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

This is to help catch OpenSSL 3.0 issues, like #67304

Author: omajid
Assignees: -
Labels:

area-Infrastructure-libraries, community-contribution

Milestone: -

@omajid omajid force-pushed the libraries-helix-ubuntu-22.04 branch from f622241 to 2e53fcc Compare March 30, 2022 15:58
@danmoseley
Copy link
Member

@agocke presumably this is increasing the cost of PR validation. Should we be swapping out a different version, instead?

@omajid
Copy link
Member Author

omajid commented Mar 30, 2022

System.Security.Cryptography.X509Certificates.Test failures on amd64 and arm64 are expected (#67304). System.Net.Http.Functional.Tests is new to me.

Edit: Filed #67353 for System.Net.Http.Functional.Tests

@danmoseley
Copy link
Member

@agocke thoughts?

@ViktorHofer
Copy link
Member

ping @dotnet/runtime-infrastructure @agocke for Dan's question above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to continue testing on Ubuntu 21.10 given that 22.04 is added to the matrix and that 21.10 will be EOL in three months (July 2022) before we even release .NET 7?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping 21.10 sounds good to me.

I am a bit surprised that the previous LTS version - 20.04 - isn't already in the test matrix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danmoseley do you remember if we intentionally left 20.04 out when we rehauled our test matrix last year?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who would be the best person to ask about ubuntu 20.04? @danmoseley @jkotas

Copy link
Member

@jkotas jkotas May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our baseline strategy is to test the lowest supported and highest supported versions, and cover the versions in the middle only if we believe that there is a particularly high risk for them. It is why Ubuntu 20.04 may be missing here since it is in the middle.

I agree that Ubuntu 21 should be deleted as part of this change.

Ubuntu 21 should be replaced by Ubuntu 22 for Linux x64 as well (this section is for Linux arm64).

If we believe that Ubuntu 20.04 is a particularly high risk, it should be ok to add it to the extra platform list. I would add it for Linux x64 only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any indication that 20.04 is high risk so I would be fine not including it and dropping 21.10. @omajid does that sound fine?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, makes sense.

22.04 is higher-risk because it's one of the earliest Linux distros that adapted OpenSSL 3.0.

@omajid omajid force-pushed the libraries-helix-ubuntu-22.04 branch 2 times, most recently from edd4a0e to 1f00ec2 Compare May 27, 2022 17:31
Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good now, @omajid. The latest update made sure to replace the middle versions 21.04 and 21.10 with the latest one, 22.04. Thanks for making the change.

But before merging, I'd like @ViktorHofer and/or @agocke do provide a sign-off too, since they have more expertise than me on this.

@agocke
Copy link
Member

agocke commented May 27, 2022

So we're swapping out 21.04 for 22.04? I think that's fine.

One thing I'd like to think about is PR vs rolling here. As we start asking questions like, what's the cost of adding this run, I think we have basically an unlimited appetite for adding runs in rolling builds.

We should start trying to make PR our highest value runs and think about moving other stuff out into rolling.

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I don't see us catching much in 21.04 that we won't catch in 22.04, and SSL 3.0 is a bigger risk category

@omajid
Copy link
Member Author

omajid commented May 27, 2022

One thing I'd like to think about is PR vs rolling here. As we start asking questions like, what's the cost of adding this run, I think we have basically an unlimited appetite for adding runs in rolling builds.

Sorry in case this is obvious to others: can you elaborate what "rolling" in this context means? Are these another set of non-PR CI builds?

@ViktorHofer
Copy link
Member

Sorry in case this is obvious to others: can you elaborate what "rolling" in this context means? Are these another set of non-PR CI builds?

Rolling builds are ones that are based of the public's main branch which happen twice per day. A PR build usually refers to a build triggered by a PR. An internal/official build is the one that is triggered when a new commit is merged into the internal main's branch.

@omajid omajid force-pushed the libraries-helix-ubuntu-22.04 branch from 1f00ec2 to a9d5033 Compare July 4, 2022 15:25
This is to help catch OpenSSL 3.0 issues.
@omajid omajid force-pushed the libraries-helix-ubuntu-22.04 branch from a9d5033 to 03df37a Compare July 4, 2022 15:27
@omajid
Copy link
Member Author

omajid commented Jul 4, 2022

I have rebased the PR to fix the merge conflicts..

@jkotas jkotas merged commit 210a507 into dotnet:main Jul 4, 2022
@jkotas
Copy link
Member

jkotas commented Jul 4, 2022

Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Aug 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Infrastructure-libraries community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants