Skip to content

config: allow .yml files to load as YAML#17455

Merged
mattklein123 merged 4 commits intomainfrom
support_yml
Jul 23, 2021
Merged

config: allow .yml files to load as YAML#17455
mattklein123 merged 4 commits intomainfrom
support_yml

Conversation

@mattklein123
Copy link
Copy Markdown
Member

This is a common source of user confusion.

Risk Level: Low
Testing: Modified tests
Docs Changes: N/A
Release Notes: Added
Platform Specific Features: N/A

This is a common source of user confusion.

Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
lakamsani added a commit to lakamsani/envoy that referenced this pull request Jul 22, 2021
…x it via envoyproxy#17455

Signed-off-by: vamsee.lakamsani <lakamsani@gmail.com>
htuch
htuch previously approved these changes Jul 23, 2021
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

TEST_P(DynamicValidationIntegrationTest, CdsProtocolOptionsRejected) {
api_filesystem_config_ = {
"test/config/integration/server_xds.bootstrap.yaml",
"test/config/integration/server_xds.bootstrap.yml",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Optional nit: technically an orthogonal test for this would be cleaner, but not that big a deal.

@htuch htuch self-assigned this Jul 23, 2021
}
if (absl::EndsWith(path, FileExtensions::get().Yaml)) {
if (absl::EndsWith(path, FileExtensions::get().Yaml) ||
absl::EndsWith(path, FileExtensions::get().Yml)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe using absl::EndsWithIgnoreCase is better, feel loose check better than strict check here. But yes, this isn't the scope of this PR also.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah this is a good idea. I will do this and a few more tests per @htuch

Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
Copy link
Copy Markdown
Member Author

@htuch @soulxu updated, thanks.

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mattklein123 mattklein123 merged commit de3f511 into main Jul 23, 2021
@mattklein123 mattklein123 deleted the support_yml branch July 23, 2021 20:15
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
This is a common source of user confusion.

Signed-off-by: Matt Klein <mklein@lyft.com>
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.

3 participants