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

Updated documentation and example configs to v3 API #10644

Merged
merged 35 commits into from
May 4, 2020
Merged

Updated documentation and example configs to v3 API #10644

merged 35 commits into from
May 4, 2020

Conversation

dmitri-d
Copy link
Contributor

@dmitri-d dmitri-d commented Apr 3, 2020

Resolves #9735

Dmitri Dolguikh added 18 commits April 3, 2020 11:27
@dmitri-d dmitri-d changed the title WIP: Update documentation and example configs to v3 API Updated documentation and example configs to v3 API Apr 8, 2020
Dmitri Dolguikh added 2 commits April 8, 2020 16:17
@htuch htuch self-assigned this Apr 10, 2020
@htuch htuch self-requested a review April 16, 2020 21:29
@dmitri-d
Copy link
Contributor Author

  • merged in latest changes from master

Copy link
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.

This is totally epic, thanks so much for taking this on, generated docs look good. I notice that version history still has v2 refs, which is what we want (no need to rewrite history); are there other files that were deliberately skipped?

Re: correctness of example fragments. I don't think it's reasonable to expect a human to manually verify the entire YAML parse of every example, but we should do a few representative ones to smoke test.

Would also like a review from @mattklein123 on the generated docs, since he generally looks at these major doc changes.

@@ -106,16 +106,18 @@ In this example we add additional context on the virtual host, and disabled the
virtual_hosts:
- name: local_service
domains: ["*"]
per_filter_config:
typed_per_filter_config:
Copy link
Member

Choose a reason for hiding this comment

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

Did you build a list of all deprecated fields to grep for? Just curious how your script works. Can you share it in a branch or gist? It's fine if it's super-hacky, just want to get a better idea of what I should be trying to validate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deprecated fields in examples were updated by hand (I did build a list of deprecated fields via grep first). I can share the script I used for updating v2 messages/fields/types/enums to v3 ones though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

See my other comment about how this can be less manual in the future, but if there is anything we should commit for future use let's definitely do that.

Copy link
Member

Choose a reason for hiding this comment

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

@dmitri-d is there some place you grepped out a list of all the deprecated field names and then checked to see if any still exist in docs? I realize there could be many false positives, but this would give me confidence that we haven't missed any (dot-the-Is, cross-the-Ts).

- socket_address:
address: localhost
port_value: 4630
load_assignment:
Copy link
Member

Choose a reason for hiding this comment

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

For the new fragments that you wrote, did you verify they worked in some kind of test harness, e.g. that this load assignment would parse? It looks reasonable, but, would be reassuring to have some kind of machine check.

Long term we want #8837, but that's outside the scope of this PR; I'd be reassured if at least they were known to parse as valid at least once.

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 did not, agree it would be a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

Ping on this one. I think it would be good to at least verify one example of each kind of change you've made to ensure there isn't typo/subtle failure. What you could do is take https://github.com/envoyproxy/envoy/blob/master/configs/google_com_proxy.v2.yaml and copy+paste in fragments with some surrounding context, verify it can be loaded with envoy -c. I don't think we need anything beyond that.

Is this a tractable amount of work or is it going to be too crazy?

@htuch
Copy link
Member

htuch commented Apr 20, 2020

https://338053-65214191-gh.circle-artifacts.com/0/generated/docs/intro/arch_overview/advanced/data_sharing_between_filters.html?highlight=per_filter_config#http-per-route-filter-configuration is an example of where per_filter_config still has untyped references. These are in text rather than code, but I think they can be spotted by grep.

@htuch
Copy link
Member

htuch commented Apr 20, 2020

Example configs at https://338053-65214191-gh.circle-artifacts.com/0/generated/docs/intro/arch_overview/security/ssl.html?highlight=verify_subject_alt_name still have verify_subject_alt_name and v2 references in type URLs.

@dmitri-d
Copy link
Contributor Author

  • fixed issues in the two files that @htuch found, and a bunch more.

Turns out I managed to miss a bunch of directories during the first pass.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks I skimmed through and agreed this is epic. I mainly have some comments on some issues we might want to open to figure out how to make this less manual from your experience. I'm pretty fearful about doc bugs from manual editing, but without doing some of the already opened issues around config checking I think it's going to be pretty difficult. :(

Thank you so much for helping with this!

@@ -106,16 +106,18 @@ In this example we add additional context on the virtual host, and disabled the
virtual_hosts:
- name: local_service
domains: ["*"]
per_filter_config:
typed_per_filter_config:
Copy link
Member

Choose a reason for hiding this comment

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

See my other comment about how this can be less manual in the future, but if there is anything we should commit for future use let's definitely do that.

@dmitri-d
Copy link
Contributor Author

  • merged in latest master

Copy link
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.

Thanks again for this epic PR. Modulo the typo comment and the question on how we can validate some representative sample in a bounded and small amount of time, this LGTM and is ready to go. Odds are we've missed one or two things, but we can launch-and-iterate.
/wait

@dmitri-d
Copy link
Contributor Author

  • updated with the latest master

Signed-off-by: Dmitri Dolguikh <[email protected]>
@dmitri-d
Copy link
Contributor Author

  • verified example configurations, fixed several of them

htuch
htuch previously approved these changes May 1, 2020
Copy link
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! Needs master merge.

@dmitri-d
Copy link
Contributor Author

dmitri-d commented May 2, 2020

  • merged in latest master

Copy link
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, will leave this for @mattklein123 to merge in case he has any more comments.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thank you! Let's ship and iterate.

@mattklein123 mattklein123 merged commit f64ae9c into envoyproxy:master May 4, 2020
@dmitri-d dmitri-d deleted the update-docs-with-api-v3 branch May 4, 2020 20:09
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.

Update documentation and example configs to v3 API
3 participants