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

Add an "AcceptEula" API to containers that use environment variables to signal EULA acceptance. #5727

Open
mitchdenny opened this issue Sep 16, 2024 · 16 comments
Assignees
Labels
area-integrations Issues pertaining to Aspire Integrations packages breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. sqlserver Issues related to SQLServer integrtions
Milestone

Comments

@mitchdenny
Copy link
Member

mitchdenny commented Sep 16, 2024

Some containers have a EULA acceptance process where you add an environment variable. Rather than adding these environment variables automatically we should provide a mechanism for developers to explicitly opt into this. This means that their first run of a particular resource will likely fail, they'll get an error and then apply another method. Here is an example of how it could work:

Container resource example

builder.AddSqlServer("sql")
       .WithAcceptEula(true);

Related resource example

builder.AddRedis("redis")
       .WithRedisInsights(c => c.WithAcceptEula());
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Sep 16, 2024
@mitchdenny mitchdenny added area-integrations Issues pertaining to Aspire Integrations packages and removed area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication labels Sep 16, 2024
@mitchdenny mitchdenny added this to the 9.0 milestone Sep 16, 2024
@mitchdenny
Copy link
Member Author

related: #5227

@davidfowl davidfowl added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Sep 16, 2024
@davidfowl
Copy link
Member

Do we know that it's the first run that fails? Today we auto accept the eula for Seq and SqlServer. These need to be undone and it has to be opted into by customers.

@mitchdenny
Copy link
Member Author

I think in the case on SQL it means every run where that environment variable isn't present.

@Alirexaa
Copy link
Contributor

related: #5227

Do you think we should apply this one in related PR? or can be a follow up?

@davidfowl
Copy link
Member

I think we should not auto accept the Eula in the redis insight PR

@davidfowl davidfowl added the sqlserver Issues related to SQLServer integrtions label Sep 18, 2024
@davidfowl
Copy link
Member

@Alirexaa do you want to do this for SQL and Seq?

@Alirexaa
Copy link
Contributor

@Alirexaa do you want to do this for SQL and Seq?

Yes, I do.

@maddymontaquila
Copy link
Member

maddymontaquila commented Sep 24, 2024

Hmmmmmmmmmm I'm thinking. Adding a new line for every single integration that needs this in your app host file is going to add up (in terms of file length/readability/repetitiveness) real quick.

The way .NET android (same issue with the android sdk) works is -
VS: First run with android bits we give you a popup that shows the EULA and says "do you accept the Android EULA" and then persist that for the user (Android SDK actually has a fiile called like eula.txt and you literally change the string in it to "TRUE" so not an env var)
VS Code: First run we fail with "Android licenses aren't accepted, run this command as admin to accept them" which is the command android ships to do the exact same thing but on the cli

Could we model that somehow? ie, when you first build, the failure is like "Accept EULA for this integration by running this cli command" (and in VS of course just do a popup)
Another thought - could we make it a build prop? <AspireAcceptIntegrationEULAs>TRUE</>?

@DamianEdwards
Copy link
Member

The NuGet experience in VS already includes prompting for EULA acceptance during install today. Can we tie into that existing experience?

@DamianEdwards
Copy link
Member

DamianEdwards commented Sep 24, 2024

I think all we need to do is update the licenses of the relevant packages to not be MIT but rather something that reflects the EULA of the underlying container image. All our packages are already marked as "Requires License Acceptance: Yes" so doing that should be enough to cover the bases here (of course IANAL).

TBC here is the UI that VS shows today when installing a package with this flag set (all our packages have this flag set):
image

I'm proposing we update the package license terms to cover the EULA of the container image being integrated.

@davidfowl
Copy link
Member

davidfowl commented Sep 29, 2024

@maddymontaquila assigning this one to you in case we can revert these methods completely.

@maddymontaquila
Copy link
Member

+1 to @DamianEdwards I think that's the move!

@joperezr
Copy link
Member

joperezr commented Oct 7, 2024

@davidfowl will take care of removing this API for RC1.

eerhardt added a commit to eerhardt/aspire that referenced this issue Oct 7, 2024
We don't want to have a custom license or agreement on our nuget packages. And this API is not ergonomic, so let's not ship this public API until we can ensure this is the way we want to go forever.

Contributes to dotnet#5727
github-actions bot pushed a commit that referenced this issue Oct 7, 2024
We don't want to have a custom license or agreement on our nuget packages. And this API is not ergonomic, so let's not ship this public API until we can ensure this is the way we want to go forever.

Contributes to #5727
eerhardt added a commit that referenced this issue Oct 8, 2024
We don't want to have a custom license or agreement on our nuget packages. And this API is not ergonomic, so let's not ship this public API until we can ensure this is the way we want to go forever.

Contributes to #5727
davidfowl pushed a commit that referenced this issue Oct 8, 2024
We don't want to have a custom license or agreement on our nuget packages. And this API is not ergonomic, so let's not ship this public API until we can ensure this is the way we want to go forever.

Contributes to #5727

Co-authored-by: Eric Erhardt <[email protected]>
@eerhardt
Copy link
Member

eerhardt commented Oct 8, 2024

The AcceptEula API was removed for RedisInsights. So each time the container is launched it will prompt the user the first time they hit the website. Using persistence (either lifetime or data volume) will preserve the settings.

Leaving this issue open to address the remaining cases, which automatically add the accept EULA environment variable, without the user explicitly accepting.

  • Sql Server
  • Seq

I'm not sure we need to address that for 9.0. @joperezr @DamianEdwards - thoughts on putting this issue on the Backlog for now?

@DamianEdwards
Copy link
Member

Using persistence (either lifetime or data volume) will preserve the settings.

@eerhardt did we add the WithDataVolume method for Redis Insights yet?

@eerhardt
Copy link
Member

eerhardt commented Oct 14, 2024

@eerhardt did we add the WithDataVolume method for Redis Insights yet?

Opened Add WithDataVolume to Redis Insights (dotnet/aspire#6299)

Moving this issue to the backlog to track if there is anything we need to do with:

@eerhardt eerhardt modified the milestones: 9.0, Backlog Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-integrations Issues pertaining to Aspire Integrations packages breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. sqlserver Issues related to SQLServer integrtions
Projects
None yet
Development

No branches or pull requests

7 participants