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

Update to httpCompression attributes #273

Merged
merged 5 commits into from
Apr 3, 2025

Conversation

remcoeissing
Copy link
Contributor

Update to how elements and attributes of httpCompression work.

@shirhatti
Copy link
Contributor

ping @bangbingsyb

Copy link
Contributor

@bangbingsyb bangbingsyb left a comment

Choose a reason for hiding this comment

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

Looks good to me except we may want to clarify the version when the change was in.

| [`dynamicTypes`](dynamictypes/index.md) | Optional element. <br><br>Specifies configuration settings for dynamic compression. |
| [`staticTypes`](statictypes/index.md) | Optional element. <br><br>Specifies configuration settings for static compression. |
| [`scheme`](scheme.md) | Optional element. <br><br>Specifies the compression scheme (Gzip or Deflate) IIS uses to compress client requests. This element can only be configured at server level. |
| [`dynamicTypes`](dynamictypes/index.md) | Optional element. <br><br>Specifies configuration settings for dynamic compression. This element can be configured at site level. |
Copy link
Contributor

@bangbingsyb bangbingsyb Nov 27, 2018

Choose a reason for hiding this comment

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

Many thanks for making the change for us!

Minor change requested: In fact, dynamicTypes and staticTypes are only allowed to be configured at site level since IIS 10.0 and above. Before IIS 10.0, they can only be configured at server level.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bangbingsyb use the suggestion mechanism so I can accept your suggestion, then I'll S&M this PR.

@Rick-Anderson Rick-Anderson requested a review from a team as a code owner April 1, 2025 21:13
@Rick-Anderson
Copy link
Contributor

@remcoeissing please review.

@@ -207,8 +207,8 @@ These attributes can only be configured at server level. Configuration at a site
| Element | Description |
| --- | --- |
| [`scheme`](scheme.md) | Optional element. <br><br>Specifies the compression scheme (Gzip or Deflate) IIS uses to compress client requests. This element can only be configured at server level. |
| [`dynamicTypes`](dynamictypes/index.md) | Optional element. <br><br>Specifies configuration settings for dynamic compression. This element can be configured at site level. |
| [`staticTypes`](statictypes/index.md) | Optional element. <br><br>Specifies configuration settings for static compression. This element can be configured at site level. |
| [`dynamicTypes`](dynamictypes/index.md) | Optional element. <br><br>Specifies configuration settings for dynamic compression. For IIS 10.0 and later, can only be configured at the site level. Versions prior to IIS 10.0, can only be configured at the server level. |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this sentence "For IIS 10.0 and later, can only be configured at the site level. Versions prior to IIS 10.0, can only be configured at the server level" indicates that dynamicTypes settings cannot be used at server level on IIS 10+. That's not correct and even default IIS 10 configuration file contains dynamicTypes at server level.

I suggest we change it to "For IIS versions prior to IIS 10.0, can only be configured at the server level. For IIS 10.0 and later, can also be configured at the site level."

| [`dynamicTypes`](dynamictypes/index.md) | Optional element. <br><br>Specifies configuration settings for dynamic compression. This element can be configured at site level. |
| [`staticTypes`](statictypes/index.md) | Optional element. <br><br>Specifies configuration settings for static compression. This element can be configured at site level. |
| [`dynamicTypes`](dynamictypes/index.md) | Optional element. <br><br>Specifies configuration settings for dynamic compression. For IIS 10.0 and later, can only be configured at the site level. Versions prior to IIS 10.0, can only be configured at the server level. |
| [`staticTypes`](statictypes/index.md) | Optional element. <br><br>Specifies configuration settings for static compression. For IIS 10.0 and later, can only be configured at the site level. Versions prior to IIS 10.0, can only be configured at the server level. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, here we should say "For IIS versions prior to IIS 10.0, can only be configured at the server level. For IIS 10.0 and later, can also be configured at the site level."

@lextm
Copy link
Contributor

lextm commented Apr 2, 2025

@Rick-Anderson @remcoeissing I added two new comments.

@Rick-Anderson Rick-Anderson requested review from lextm and wadepickett and removed request for lextm April 2, 2025 20:34
@wadepickett
Copy link
Contributor

wadepickett commented Apr 2, 2025

I started to make some very minor edits but then reverted them. I thought I spotted a lextm's change that hadn't been pulled in yet, and received an odd server error, so backed out. Was just updating the ms.date, nothing crucial.

@wadepickett wadepickett self-requested a review April 2, 2025 22:43
Copy link
Contributor

@wadepickett wadepickett left a comment

Choose a reason for hiding this comment

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

Looks good to me. Consider updating the ms.date from 2016 to the current since there were some significant changes.

@Rick-Anderson Rick-Anderson merged commit f36126b into MicrosoftDocs:main Apr 3, 2025
2 checks passed
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.

6 participants