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

Review template rendering, and release authoring-related docs #813

Merged
merged 11 commits into from
Dec 14, 2023

Conversation

bgandon
Copy link
Contributor

@bgandon bgandon commented Nov 10, 2023

Hello Bosh mates,

This is a big PR after a large review of several pieces of docs related to release development. As a release author and as a trainer (facing the trainees legitimate “surprise” when reading the Bosh docs), I wanted to complete and update these for years, so I’m glad I can contribute something today.

Individual commits are breaking the changes into smaller consistent chunks. This might ease the review.

I’ve went quite far in documenting the spec object in the evaluation context for ERB templates. In fact, many exposed properties should not be there at all. We can discuss which ones, and I can submit a PR for that refactoring in bosh core.

I’ve also added a “caveat” about the obviously missing spec.job_name property, that would be very useful for release authors, even though not strictly necessary. Indeed, that would allow writing ERB snippets that can just be used with pure copy/paste, no editing, and that would be cool. Being cool does matter.

Cheers

@rkoster rkoster requested review from a team, klakin-pivotal and nouseforaname and removed request for a team November 16, 2023 16:02
Copy link
Contributor

@ystros ystros left a comment

Choose a reason for hiding this comment

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

Thanks for these docs updates! In addition to the more thorough field documentation for templates and spec files, I like the emphasis of moving folks to use BPM instead of using control scripts.

I spotted several typos; I may not have spotted everything, so it may be good to run these pages through a spell checker. Otherwise, looks good overall!

content/jobs.md Outdated Show resolved Hide resolved
content/jobs.md Outdated Show resolved Hide resolved
content/jobs.md Outdated Show resolved Hide resolved
content/jobs.md Outdated Show resolved Hide resolved
content/jobs.md Show resolved Hide resolved
content/jobs.md Outdated Show resolved Hide resolved
@bgandon
Copy link
Contributor Author

bgandon commented Nov 23, 2023

Thanks for the review @ystros 👍 Unfortunately didn’t see your comments until now, but will fix the typos soon!

@bgandon
Copy link
Contributor Author

bgandon commented Nov 28, 2023

Thanks @ystros I’ fixed the typos.

Please note that my last two commit have introduced new remarks

Thanks for reviewing these two additions!

@jpalermo jpalermo requested a review from ystros November 30, 2023 15:45
Copy link
Contributor

@ystros ystros left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Documenting how to get network names via our unusual coupling with OpenStruct makes me nervous (making it a de-facto supported mechanism), but it accurately reflects reality right now. The docs can be updated later if we ever change that coupling or provide a better mechanism for getting network names.

lgtm!

@beyhan
Copy link
Member

beyhan commented Dec 7, 2023

Thanks @bgandon!

@rkoster rkoster merged commit b40ce5c into master Dec 14, 2023
1 check passed
@rkoster rkoster deleted the review-template-rendering branch December 14, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants