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

Set assembly metadata with build configuration for AppHost and apply to projects in CLI mode #3961

Merged
merged 6 commits into from
May 16, 2024

Conversation

danegsta
Copy link
Member

@danegsta danegsta commented Apr 25, 2024

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Apr 25, 2024
@@ -88,7 +88,7 @@ public async Task EndpointPortsExecutableNotReplicatedProxiedPortSetNoTargetPort
Assert.True(dcpExe.TryGetAnnotationAsObjectList<ServiceProducerAnnotation>(CustomResource.ServiceProducerAnnotation, out var spAnnList));
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to test this change?

Copy link
Member

Choose a reason for hiding this comment

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

@radical discovered this issue by writing an end-to-end test with his workload infrastructure. That is not in yet, but he is aiming on pushing a PR up next week.

Copy link
Member

Choose a reason for hiding this comment

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

Tests pass with the changes from this PR added to my workload-tests branch.

@joperezr
Copy link
Member

@danegsta is this PR stale? should it be closed?

@danegsta
Copy link
Member Author

@joperezr this is likely stale at this point; did we get a different workaround for the release mode problem merged?

@joperezr
Copy link
Member

I believe so but not 100% sure, I thought @mitchdenny came up with a different fix? I can test real quick

@joperezr
Copy link
Member

Just tested this and it is in fact not yet fixed. Looks like this is one thing we'll want to take for 8.1. There was some conversation in chat around using special attributes, assuming all of the feedback is already incorporated with this PR?

@danegsta
Copy link
Member Author

Yeah, it's been updated to honor the built-in AssemblyConfigurationAttribute the SDK applies, rather than rolling our own assembly attribute via targets.

@danegsta
Copy link
Member Author

I'll merge main into the PR branch to kick off a fresh build, just to make sure nothing has drifted far enough to cause issues since the last build.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

These changes LGTM

@danegsta danegsta merged commit 21a5888 into dotnet:main May 16, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants