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

Fix and Edit build time OpenAPI/ra/b #33361

Merged
merged 74 commits into from
Oct 14, 2024

Conversation

Rick-Anderson
Copy link
Contributor

@Rick-Anderson Rick-Anderson commented Aug 14, 2024

@Rick-Anderson Rick-Anderson marked this pull request as draft August 15, 2024 03:05
Copy link
Contributor Author

@Rick-Anderson Rick-Anderson left a comment

Choose a reason for hiding this comment

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

@captainsafia this is just a quick edit. It likely has lots of error I'll find tomorrow. I do have a couple questions. Note this branch fixes all the build errors in your branch. This will merge into your branch so you should get credit for the commit to main.

Comment on lines 5 to 8
if (Assembly.GetEntryAssembly()?.GetName().Name != "GetDocument.Insider")
{
// builder.Services.AddDefaults();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

builder.Services.AddDefaults(); generates the error:
'IServiceCollection' does not contain a definition for 'AddDefaults'

Copy link
Member

Choose a reason for hiding this comment

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

This is my bad...I had trouble figuring out what would be a good illustration of a setup here.

Maybe we can do something that configures an EF Core DB context?

services.AddDbContext<MyContext>(options =>
                options.UseSqlServer(Configuration.GetConnectionString("DefaultConnection")));

We'd have to add a definition for MyContext and the package references for EF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@captainsafia @martincostello

This is my bad...I had trouble figuring out what would be a good illustration of a setup here.

Maybe we can do something that configures an EF Core DB context?

services.AddDbContext<MyContext>(options =>
                options.UseSqlServer(Configuration.GetConnectionString("DefaultConnection")));

We'd have to add a definition for MyContext and the package references for EF

I have:

Comment on lines 9 to 14
//if (Assembly.GetEntryAssembly()?.GetName().Name != "GetDocument.Insider")
//{
builder.Services.AddDbContext<ControllerApiContext>(options =>
options.UseSqlServer(builder.Configuration.GetConnectionString("ControllerApiContext")
?? throw new InvalidOperationException("Connection string 'ControllerApiContext' not found.")));
//}
Copy link
Contributor Author

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 connection string or sql info when //if (Assembly.GetEntryAssembly()?.GetName().Name != "GetDocument.Insider") is commented out. I don't see any difference in the generated OpenAPI file with or without the build check.

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 connection string or sql info when //if (Assembly.GetEntryAssembly()?.GetName().Name != "GetDocument.Insider") is commented out

I believe this is because there isn't a connection string configured in the appsettings for this project. See changes in c86bab7.

I don't see any difference in the generated OpenAPI file with or without the build check.

In this case, that is the expected behavior. 👍🏽

@Rick-Anderson Rick-Anderson marked this pull request as draft August 22, 2024 23:07
@Rick-Anderson Rick-Anderson marked this pull request as ready for review September 3, 2024 23:36
@Rick-Anderson Rick-Anderson changed the title Fix and Edit build time OpenAPI/ra Fix and Edit build time OpenAPI/ra/b Sep 3, 2024
@Rick-Anderson
Copy link
Contributor Author

@captainsafia can you look this over and let me know what needs to change/delete before I can S&M it into your #33359? I'll then fix the merge conflicts in your #33359.

@mikekistler mikekistler self-requested a review September 19, 2024 21:33
Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

This looks good, but I want to suggest an alternative organization:

  • Broadening the "Generating OpenAPI documents at build time" to cover all of the "Generating OpenAPI documents" content. That would involve pulling the first two sections of the "Work with OpenAPI documents" (Package Installation and Configuring OpenAPI Generation) over into this doc, leaving the "Work with" doc more clearly focused on how to add metadata. If we did this I think the Generating OpenAPI documents topic would then come before "Working with ...".

Up to you if you think that's a better approach. I'll approve now since I think all the content here is solid.

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Let's merge this and continue iterating on the other PR -- I think this is in good enough shape.

@Rick-Anderson Rick-Anderson merged commit 2698d41 into openapi-build-docs Oct 14, 2024
2 of 3 checks passed
@Rick-Anderson Rick-Anderson deleted the ra/Safia/add-build-time-OpenAPI branch October 14, 2024 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants