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 aspnet-request-servervariable and aspnet-request-item layout renders #811

Merged
merged 16 commits into from
Jul 6, 2022

Conversation

bakgerman
Copy link
Contributor

@bakgerman bakgerman commented Jul 3, 2022

Most of the server variables can be done with other layout render, but some like APP_POOL_ID are only in server variable collection. See https://docs.microsoft.com/en-us/previous-versions/iis/6.0-sdk/ms524602(v=vs.90)

(I change the title of the pull request to match the actual commit now).

@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2022

Codecov Report

Merging #811 (5f994cf) into master (a35f036) will decrease coverage by 0%.
The diff coverage is 54%.

❗ Current head 5f994cf differs from pull request most recent head cfc38c8. Consider uploading reports for the commit cfc38c8 to get more accurate results

@@          Coverage Diff          @@
##           master   #811   +/-   ##
=====================================
- Coverage      70%    69%   -0%     
=====================================
  Files          62     64    +2     
  Lines        1146   1159   +13     
  Branches      289    294    +5     
=====================================
+ Hits          797    801    +4     
- Misses        225    231    +6     
- Partials      124    127    +3     
Impacted Files Coverage Δ
...ayoutRenderers/AspNetRequestValueLayoutRenderer.cs 97% <ø> (ø)
...utRenderers/AspNetHttpContextItemLayoutRenderer.cs 50% <50%> (ø)
...erers/AspNetRequestServerVariableLayoutRenderer.cs 60% <60%> (ø)
...utRenderers/AspNetRequestDurationLayoutRenderer.cs 58% <0%> (-7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a35f036...cfc38c8. Read the comment docs.

@snakefoot
Copy link
Contributor

Maybe introduce a dedicated layoutrenderer. Ex. ${aspnet-request-servervariable} to follow the guidance from the wiki

@bakgerman
Copy link
Contributor Author

Yes I see, that makes sense. I will do that.

@pull-request-size pull-request-size bot added size/XL and removed size/M labels Jul 4, 2022
@bakgerman
Copy link
Contributor Author

bakgerman commented Jul 4, 2022

Added aspnet-request-servervariable and also added aspnet-request-item since I did not see that one either.
Should I make the single parameter for both of these [RequiredParameter]?

@bakgerman bakgerman changed the title Restore Server Variables for ASP_NET_CORE3 Add aspnet-request-servervariable and aspnet-request-item layout renders Jul 4, 2022
@pull-request-size pull-request-size bot added size/L and removed size/XL labels Jul 5, 2022
@bakgerman
Copy link
Contributor Author

Fixing a unit test

@bakgerman
Copy link
Contributor Author

nuget.org has decided that github.com makes too many requests. This is the error from Appveyor build:

Retrying 'FindPackagesByIdAsync' for source 'https://api.nuget.org/v3-flatcontainer/microsoft.sourcelink.common/index.json'.
Response status code does not indicate success: 429 (Too Many Requests).
Failed to download package 'Microsoft.Build.Tasks.Git.1.1.1' from 'https://api.nuget.org/v3-flatcontainer/microsoft.build.tasks.git/1.1.1/microsoft.build.tasks.git.1.1.1.nupkg'.

@sonarcloud
Copy link

sonarcloud bot commented Jul 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

94.4% 94.4% Coverage
0.0% 0.0% Duplication

@snakefoot snakefoot merged commit 3a00ceb into NLog:master Jul 6, 2022
@snakefoot
Copy link
Contributor

Thank you again for being patient with me, and resolving all my nitpicks.

@snakefoot
Copy link
Contributor

Hrmm just discovered this baby AspNetItemValueLayoutRenderer. Guess something must be merged.

@bakgerman
Copy link
Contributor Author

Thank you for being patient with me also.

@bakgerman
Copy link
Contributor Author

I did not see that AspNetItemValueLayoutRenderer either. (That is where the proper naming helps).

@snakefoot
Copy link
Contributor

Created #814

@bakgerman bakgerman deleted the addl-layout-renders branch July 6, 2022 14:30
@snakefoot
Copy link
Contributor

I did not see that AspNetItemValueLayoutRenderer either. (That is where the proper naming helps).

Yes naming is a hard dicipline. Always a good idea to ask a friend.

@snakefoot snakefoot added this to the 5.1.0 milestone Jul 6, 2022
@snakefoot
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants