Skip to content
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

[Containers] Fix parsing and error reporting of ports that lack port type metadata #43551

Merged

Conversation

baronfel
Copy link
Member

@baronfel baronfel commented Sep 18, 2024

Description

Users that attempted to use the SDK to declare open ports in their containers had a very difficult time using the documented pathways due to bugs in the validation and error reporting. The reported error didn't match the user's actual data problem, and the suggested fix wouldn't resolve the users' problem.

Customer Impact

Customers could workaround the issue by fully-specifying the Port to declare, but this was hard to discover and the docs, errors, and user expectations for MSBuild didn't guide users to success.

Regression

No, this has been a problem since the first version of the feature

Risk

Low - we have very broad test coverage, and in fact had codified bad behavior in the tests. Making the tests reflect the actual desired user experience verified the error and the fix very easily.

Technical details

Fixes dotnet/sdk-container-builds#596

There were two problems:

  • non-existent metadata from MSBuild is an empty string, not null, so the 'is a port present' check needed to take this into account
  • the flags error enum needs to be made of all-non-zero members or else Enum.HasFlag will always return true for the zero value. this is documented and understandable behavior (since HasFlag is shorthand for (value & target) == target, and anything AND-ed with 0 will be 0.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Containers Related to dotnet SDK containers functionality untriaged Request triage from a team member labels Sep 18, 2024
@baronfel baronfel changed the base branch from release/9.0.1xx to main September 23, 2024 14:50
@baronfel baronfel requested review from a team, BillHiebert, tmat and arunchndr as code owners September 23, 2024 14:50
@baronfel baronfel removed request for a team, tmat, BillHiebert and arunchndr September 23, 2024 14:50
@baronfel baronfel changed the base branch from main to release/9.0.1xx September 23, 2024 14:50
@baronfel
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@baronfel baronfel requested a review from a team September 23, 2024 14:59
@baronfel
Copy link
Member Author

Sanity check - @marcpopMSFT this was targeting 9.0.100 because it's a bug in validation and is hard for users to understand. Guessing we need Tactics approval at this point?

@baronfel baronfel changed the title fix parsing of ports that lack port type metadata [Containers] Fix parsing and error reporting of ports that lack port type metadata Sep 26, 2024
@baronfel
Copy link
Member Author

baronfel commented Oct 6, 2024

Closing as @surayya-MS got this merged in already in teh 8.0.4xx prerequisite PR she got merged!

@baronfel baronfel closed this Oct 6, 2024
@baronfel
Copy link
Member Author

baronfel commented Oct 6, 2024

wrong PR - this one got approved by servicing.

@baronfel baronfel reopened this Oct 6, 2024
@baronfel baronfel merged commit 59a4222 into dotnet:release/9.0.1xx Oct 6, 2024
31 checks passed
@baronfel
Copy link
Member Author

baronfel commented Oct 6, 2024

/backport to main

@baronfel
Copy link
Member Author

baronfel commented Oct 6, 2024

/backport to release/8.0.4xx

Copy link
Contributor

github-actions bot commented Oct 6, 2024

Started backporting to main: https://github.com/dotnet/sdk/actions/runs/11205328409

Copy link
Contributor

github-actions bot commented Oct 6, 2024

Started backporting to release/8.0.4xx: https://github.com/dotnet/sdk/actions/runs/11205329248

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Containers Related to dotnet SDK containers functionality Servicing-approved untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants