Skip to content

Make Auth Samples Project Ref #6557

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jan 15, 2019
Merged

Make Auth Samples Project Ref #6557

merged 8 commits into from
Jan 15, 2019

Conversation

jkotalik
Copy link
Contributor

@Tratcher I know you were discussing where to put these samples. Do you want them in Security?

@Tratcher
Copy link
Member

Security sounds good. @HaoK do you care about separating the Identity sample?

@jkotalik jkotalik requested a review from JunTaoLuo January 11, 2019 01:10
natemcmaster
natemcmaster reviewed Jan 11, 2019
View reviewed changes
Copy link
Contributor

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

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

Just FYI, naming the parent folder src/Security/AuthSamples/ is going to cause this line to think everything inside is a sample, including the test projects.

https://github.com/aspnet/AspNetCore/blob/3b67abecbfc9933d73bf26c8b88bd7167cf471b9/Directory.Build.props#L29

What if we tweaked the layout slightly? Thoughts @Tratcher ?

src/
  Security/
    test/
       FunctionalTests/
          AuthSamples.FunctionalTests.csproj
    testassets/
       ClaimsTransformation/
       Cookies/

@jkotalik jkotalik force-pushed the jkotalik/authSamples21 branch from ba905b1 to 26a2563 Compare January 11, 2019 19:04
JunTaoLuo
JunTaoLuo reviewed Jan 11, 2019
View reviewed changes
src/Security/AuthSamples.sln
@@ -0,0 +1,124 @@
Microsoft Visual Studio Solution File, Format Version 12.00
Copy link
Contributor

Choose a reason for hiding this comment

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

There needs to be a lot more project references to be added here to fix all the NU1105 errors.

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 thought we didn't need this if we have packages? I don't see any failures when I build the solution right now.

@HaoK
Copy link
Member

HaoK commented Jan 11, 2019

Nah these can all live together under security

@Tratcher
Copy link
Member

@natemcmaster these are intentionally built as user facing samples, they are not assets of the tests. The tests are only to verify the samples.

@jkotalik
Copy link
Contributor Author

@Tratcher I agree with you. I'll add <IsTestAssetProject>true</IsTestAssetProject> to all projects

@Tratcher Tratcher requested a review from HaoK January 11, 2019 23:54
natemcmaster
natemcmaster approved these changes Jan 12, 2019
View reviewed changes
Copy link
Contributor

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

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

One nit, otherwise LGTM.

@jkotalik jkotalik merged commit e751db0 into release/2.1 Jan 15, 2019
@jkotalik jkotalik deleted the jkotalik/authSamples21 branch January 15, 2019 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natemcmaster natemcmaster natemcmaster approved these changes

@Tratcher Tratcher Tratcher approved these changes

@JunTaoLuo JunTaoLuo JunTaoLuo approved these changes

@HaoK HaoK HaoK approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jkotalik @Tratcher @HaoK @JunTaoLuo @natemcmaster