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

docs: added docs for config resolution flow #4978

Merged
merged 2 commits into from
Aug 28, 2023
Merged

Conversation

thsig
Copy link
Collaborator

@thsig thsig commented Aug 23, 2023

What this PR does / why we need it:

This should be useful for contributors when debugging config resolution issues or adding features relating to templating (at least to get a high-level orientation and figure out where to start).

@thsig thsig force-pushed the config-resolution-docs branch 2 times, most recently from b3f2d1e to abb7255 Compare August 24, 2023 08:05
vvagaytsev

This comment was marked as outdated.

Copy link
Collaborator

@vvagaytsev vvagaytsev 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! 👍
I've left a few comments, please check.

@thsig
Copy link
Collaborator Author

thsig commented Aug 24, 2023

Should be good to go now—I addressed most of the comments (and skipped some of them).

Thanks for reading through this!

@eysi09
Copy link
Collaborator

eysi09 commented Aug 26, 2023

This is great! I see myself looking this up multiple times.

Out of scope for this PR but I'm wondering if we should collect all these in a single place, e.g. a top-level dir called contributing as opposed to co-locating with code.

(I know co-locating is a good thing in general, but maybe high level architectural docs like this are an exception.)

@thsig thsig force-pushed the config-resolution-docs branch 3 times, most recently from 60b643b to c8d0fff Compare August 28, 2023 11:26
@thsig
Copy link
Collaborator Author

thsig commented Aug 28, 2023

This is great! I see myself looking this up multiple times.

Out of scope for this PR but I'm wondering if we should collect all these in a single place, e.g. a top-level dir called contributing as opposed to co-locating with code.

(I know co-locating is a good thing in general, but maybe high level architectural docs like this are an exception.)

Good idea! I'll do that.

This should be useful for contributors when debugging config resolution
issues or adding features relating to templating (at least to get a
high-level orientation and figure out where to start).
@thsig
Copy link
Collaborator Author

thsig commented Aug 28, 2023

I read this through again and made a couple of edits and clarifications. Should be good to go.

@worldofgeese
Copy link
Contributor

@thsig my only critique is I'm not sure I understand why we're embedding docs in core/src. Shouldn't this be surfaced elsewhere like our actual docs? I could see this as of interest for outside contributing developers. If the docs are inside core/src discoverability seems poor but maybe I'm missing something.

@thsig
Copy link
Collaborator Author

thsig commented Aug 28, 2023

Yeah, makes sense. I've moved these to contributor-docs at the top level (and also added a mention of this directory to the CONTRIBUTING.md guide).

Copy link
Contributor

@worldofgeese worldofgeese left a comment

Choose a reason for hiding this comment

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

I made one suggestion but this otherwise looks like a fantastic resource for folks looking to understand config resolution.

@worldofgeese worldofgeese self-requested a review August 28, 2023 13:05
@thsig thsig added this pull request to the merge queue Aug 28, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 28, 2023
@thsig thsig added this pull request to the merge queue Aug 28, 2023
Merged via the queue into main with commit d5d006e Aug 28, 2023
41 checks passed
@thsig thsig deleted the config-resolution-docs branch August 28, 2023 16:03
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.

None yet

4 participants