Add `GetEnvironmentVariableValuesAsync` method to ResourceExtensions #4530

Alirexaa · 2024-06-15T19:32:14Z

This change is based on this EnvironmentVariableEvaluator.GetEnvironmentVariablesAsync method
with little change.

the difference between the issue Proposed API and this commit:

The issue Proposed API :

public static class ResourceExtensions
{
   public static Task<IReadOnlyDictionary<string, string?>> GetEnvironmentVariableValuesAsync(this IResourceWithEnvironment resource);
}

The commit Proposed API :

public static class ResourceExtensions
{
  public static async ValueTask<IReadOnlyDictionary<string, string>> GetEnvironmentVariableValuesAsync(this IResourceWithEnvironment resource,
    DistributedApplicationOperation applicationOperation = DistributedApplicationOperation.Run);
}

Microsoft Reviewers: Open in CodeFlow

…net#4464

mitchdenny · 2024-06-17T00:43:42Z

src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs

+    /// </summary>
+    /// <param name="resource">The resource to get the environment variables from.</param>
+    /// <param name="applicationOperation">The context in which the AppHost is being executed</param>
+    /// <returns>The environment variables retrieved from the resource.</returns>


Can we get some <remarks> added which cover off where this method is useful and considerations around the sync nature. As well as one or two <example> sections which cover off its usage.

I'm not sure this needs its own conceptual document, but the following could be expanded:

https://learn.microsoft.com/en-us/dotnet/aspire/fundamentals/testing

Could you create a docs issue to capture that?

mitchdenny · 2024-06-17T00:47:04Z

@Alirexaa this is looking good. Just want to make sure we capture the doc requirement and make sure we expand the XML docs a bit for this change.

We need some covering tests as well.

RussKie · 2024-06-18T23:09:41Z

/azp run

azure-pipelines · 2024-06-18T23:09:52Z

Azure Pipelines successfully started running 1 pipeline(s).

mitchdenny · 2024-06-19T03:03:43Z

@Alirexaa just to be clear we want the XML doc comments in this PR :)

Alirexaa · 2024-06-19T07:04:35Z

@Alirexaa just to be clear we want the XML doc comments in this PR :)

sure 😄

Could you create a docs issue to capture that?

@mitchdenny, what did you mean by that?

DamianEdwards · 2024-06-20T22:22:34Z

@Alirexaa

@mitchdenny, what did you mean by that?

Just that can you please log an issue in dotnet/docs-aspire to ensure the new documentation requirement is captured.

src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs

Co-authored-by: Damian Edwards <damian@damianedwards.com>

Co-authored-by: Ankit Jain <radical@gmail.com>

JamesNK · 2024-07-03T05:30:23Z

@Alirexaa We have a deadline for 8.1 coming in a couple of weeks and we'd like to include this feature. Do you have time to look at the changes soon? If not, I can help out by addressing some feedback on your branch.

Alirexaa · 2024-07-03T14:25:57Z

@Alirexaa We have a deadline for 8.1 coming in a couple of weeks and we'd like to include this feature. Do you have time to look at the changes soon? If not, I can help out by addressing some feedback on your branch.

Yes, I have time, but I need some examples to write tests for the following cases :

src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs

eerhardt · 2024-07-18T22:03:22Z

tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs

+            env =>
+            {
+                Assert.Equal("{ElasticPassword.value}", env.Value);
+                Assert.False(string.IsNullOrEmpty(env.Value));


We should verify the Value is the correct parameter reference in publish mode, not just that it isn't null or empty.

Reactivating. This hasn't been resolved yet.

tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs

radical · 2024-07-18T22:06:25Z

tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs

+         .WithEnvironment("xpack.security.enabled", "true")
+         .WithEnvironment(context =>
+         {
+             context.EnvironmentVariables["ELASTIC_PASSWORD"] = "123456";


To avoid credscan failure:

Suggested change

context.EnvironmentVariables["ELASTIC_PASSWORD"] = "123456";

context.EnvironmentVariables["ELASTIC_PASSWORD"] = "p@ssw0rd1";

tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs

Co-authored-by: Ankit Jain <radical@gmail.com>

mitchdenny · 2024-07-21T11:14:30Z

@joperezr we are going to want to get this one in for 8.1

eerhardt

LGTM

eerhardt · 2024-07-22T00:02:32Z

tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs

+         .WithEnvironment("xpack.security.enabled", "true")
+         .WithEnvironment(context =>
+         {
+             context.EnvironmentVariables["ELASTIC_PASSWORD"] = "p@ssw0rd1";


I think we are still going to get cred scan failures here, but we can try it.

Will see. @radical was saying that they were in a suppression list.

There are 2 cred scan checks, and my understanding is that only one of them respects the suppression list.

mitchdenny · 2024-07-22T00:32:49Z

/backport to release/8.1

github-actions · 2024-07-22T00:33:01Z

Started backporting to release/8.1: https://github.com/dotnet/aspire/actions/runs/10032596995

github-actions · 2024-07-22T00:33:05Z

@mitchdenny backporting to release/8.1 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Add to ResourceExtensions ```GetEnvironmentVariableValuesAsync``` #4464
Using index info to reconstruct a base tree...
M	src/Aspire.Hosting/PublicAPI.Unshipped.txt
Falling back to patching base and 3-way merge...
Auto-merging src/Aspire.Hosting/PublicAPI.Unshipped.txt
CONFLICT (content): Merge conflict in src/Aspire.Hosting/PublicAPI.Unshipped.txt
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Add to ResourceExtensions ```GetEnvironmentVariableValuesAsync``` #4464
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2024-07-22T00:33:05Z

@mitchdenny an error occurred while backporting to release/8.1, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

…ons (#4530) * Add to ResourceExtensions ```GetEnvironmentVariableValuesAsync``` #4464 * add example and remarks * add test * Update src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs Co-authored-by: Damian Edwards <damian@damianedwards.com> * Update src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs Co-authored-by: Ankit Jain <radical@gmail.com> * Update src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs Co-authored-by: Ankit Jain <radical@gmail.com> * add more tests,add suggested code * Tweak API based on Eric's feedback. * Use a different example. * Update src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs Co-authored-by: Ankit Jain <radical@gmail.com> * Remove password from example. * Indentation fixes. * Update tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs Co-authored-by: Ankit Jain <radical@gmail.com> * Update tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs Co-authored-by: Ankit Jain <radical@gmail.com> * PR feedback. * Namespace fix. --------- Co-authored-by: Mitch Denny <midenn@microsoft.com> Co-authored-by: Damian Edwards <damian@damianedwards.com> Co-authored-by: Ankit Jain <radical@gmail.com> Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com> Co-authored-by: Mitch Denny <mitchdenny@outlook.com>

…ons (#4530) (#5005) * Add to ResourceExtensions ```GetEnvironmentVariableValuesAsync``` #4464 * add example and remarks * add test * Update src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs * Update src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs * Update src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs * add more tests,add suggested code * Tweak API based on Eric's feedback. * Use a different example. * Update src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs * Remove password from example. * Indentation fixes. * Update tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs * Update tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs * PR feedback. * Namespace fix. --------- Co-authored-by: Alireza Baloochi <alireza.baloochi1380@gmail.com> Co-authored-by: Damian Edwards <damian@damianedwards.com> Co-authored-by: Ankit Jain <radical@gmail.com> Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

Add to ResourceExtensions GetEnvironmentVariableValuesAsync dot…

dabab87

…net#4464

dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jun 15, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 15, 2024

Merge branch 'main' into ResourceExtensions

0837fd0

Alirexaa changed the title ~~Add to ResourceExtensions GetEnvironmentVariableValuesAsync #4464~~ Add GetEnvironmentVariableValuesAsync method to ResourceExtensions Jun 15, 2024

mitchdenny reviewed Jun 17, 2024

View reviewed changes

Alirexaa mentioned this pull request Jun 17, 2024

Docs: Add docs for GetEnvironmentVariableValuesAsync #4551

Closed

mitchdenny self-assigned this Jun 19, 2024

Merge branch 'main' into ResourceExtensions

07b576a

Alirexaa mentioned this pull request Jun 25, 2024

Expand testing docs dotnet/docs-aspire#1151

Closed

IEvangelist mentioned this pull request Jun 26, 2024

Update Aspire.Hosting.Testing doc to cover new changes in 8.1 dotnet/docs-aspire#1114

Closed

4 tasks

Alirexaa added 2 commits June 27, 2024 13:13

add example and remarks

1d0a3cc

add test

6d4c568

DamianEdwards reviewed Jun 27, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs Outdated Show resolved Hide resolved

DamianEdwards reviewed Jun 27, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs Outdated Show resolved Hide resolved

DamianEdwards reviewed Jun 27, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs Outdated Show resolved Hide resolved

radical reviewed Jun 27, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs Outdated Show resolved Hide resolved

src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs Outdated Show resolved Hide resolved

src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs Show resolved Hide resolved

Alirexaa and others added 3 commits June 27, 2024 23:44

Update src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs

95fbc40

Co-authored-by: Damian Edwards <damian@damianedwards.com>

Update src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs

8624afa

Co-authored-by: Ankit Jain <radical@gmail.com>

Update src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs

de3cb95

Co-authored-by: Ankit Jain <radical@gmail.com>

This comment was marked as outdated.

Sign in to view

add more tests,add suggested code

8214b94

mitchdenny and others added 2 commits July 19, 2024 00:23

Use a different example.

c13cb4f

Merge remote-tracking branch 'upstream/main' into ResourceExtensions

28a36e5

radical reviewed Jul 18, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs Outdated Show resolved Hide resolved

radical reviewed Jul 18, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs Outdated Show resolved Hide resolved

radical reviewed Jul 18, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs Outdated Show resolved Hide resolved

eerhardt reviewed Jul 18, 2024

View reviewed changes

radical reviewed Jul 18, 2024

View reviewed changes

tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs Outdated Show resolved Hide resolved

radical reviewed Jul 18, 2024

View reviewed changes

tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs Outdated Show resolved Hide resolved

tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs Outdated Show resolved Hide resolved

mitchdenny and others added 3 commits July 19, 2024 09:40

Update src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs

fedacd2

Co-authored-by: Ankit Jain <radical@gmail.com>

Remove password from example.

c4237c3

Indentation fixes.

80950f7

mitchdenny requested review from eerhardt and radical July 18, 2024 23:48

mitchdenny and others added 4 commits July 21, 2024 16:30

Update tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs

6ca31f9

Co-authored-by: Ankit Jain <radical@gmail.com>

Update tests/Aspire.Hosting.Tests/ResourceExtensionsTests.cs

94f524c

Co-authored-by: Ankit Jain <radical@gmail.com>

PR feedback.

3a0ac4e

Namespace fix.

15b4ba8

eerhardt approved these changes Jul 22, 2024

View reviewed changes

mitchdenny merged commit e3ea763 into dotnet:main Jul 22, 2024
9 checks passed

mitchdenny mentioned this pull request Jul 22, 2024

[release/8.1] Backport: Add GetEnvironmentVariableValuesAsync method to ResourceExtens… #5005

Merged

Alirexaa deleted the ResourceExtensions branch July 22, 2024 16:45

github-actions bot locked and limited conversation to collaborators Aug 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `GetEnvironmentVariableValuesAsync` method to ResourceExtensions #4530

Add `GetEnvironmentVariableValuesAsync` method to ResourceExtensions #4530

Alirexaa commented Jun 15, 2024 •

edited

Loading

mitchdenny Jun 17, 2024

mitchdenny commented Jun 17, 2024

RussKie commented Jun 18, 2024

azure-pipelines bot commented Jun 18, 2024

mitchdenny commented Jun 19, 2024

Alirexaa commented Jun 19, 2024

DamianEdwards commented Jun 20, 2024

This comment was marked as outdated.

JamesNK commented Jul 3, 2024 •

edited

Loading

Alirexaa commented Jul 3, 2024 •

edited

Loading

eerhardt Jul 18, 2024

eerhardt Jul 18, 2024

radical Jul 18, 2024

mitchdenny commented Jul 21, 2024

eerhardt left a comment

eerhardt Jul 22, 2024

mitchdenny Jul 22, 2024

eerhardt Jul 22, 2024

mitchdenny commented Jul 22, 2024

github-actions bot commented Jul 22, 2024

github-actions bot commented Jul 22, 2024

github-actions bot commented Jul 22, 2024

	context.EnvironmentVariables["ELASTIC_PASSWORD"] = "123456";
	context.EnvironmentVariables["ELASTIC_PASSWORD"] = "p@ssw0rd1";

Add GetEnvironmentVariableValuesAsync method to ResourceExtensions #4530

Add GetEnvironmentVariableValuesAsync method to ResourceExtensions #4530

Conversation

Alirexaa commented Jun 15, 2024 • edited Loading

Microsoft Reviewers: Open in CodeFlow

mitchdenny Jun 17, 2024

Choose a reason for hiding this comment

mitchdenny commented Jun 17, 2024

RussKie commented Jun 18, 2024

azure-pipelines bot commented Jun 18, 2024

mitchdenny commented Jun 19, 2024

Alirexaa commented Jun 19, 2024

DamianEdwards commented Jun 20, 2024

This comment was marked as outdated.

JamesNK commented Jul 3, 2024 • edited Loading

Alirexaa commented Jul 3, 2024 • edited Loading

eerhardt Jul 18, 2024

Choose a reason for hiding this comment

eerhardt Jul 18, 2024

Choose a reason for hiding this comment

radical Jul 18, 2024

Choose a reason for hiding this comment

mitchdenny commented Jul 21, 2024

eerhardt left a comment

Choose a reason for hiding this comment

eerhardt Jul 22, 2024

Choose a reason for hiding this comment

mitchdenny Jul 22, 2024

Choose a reason for hiding this comment

eerhardt Jul 22, 2024

Choose a reason for hiding this comment

mitchdenny commented Jul 22, 2024

github-actions bot commented Jul 22, 2024

github-actions bot commented Jul 22, 2024

github-actions bot commented Jul 22, 2024

Add `GetEnvironmentVariableValuesAsync` method to ResourceExtensions #4530

Add `GetEnvironmentVariableValuesAsync` method to ResourceExtensions #4530

Alirexaa commented Jun 15, 2024 •

edited

Loading

JamesNK commented Jul 3, 2024 •

edited

Loading

Alirexaa commented Jul 3, 2024 •

edited

Loading