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

Build System Docs #7544

Merged
merged 7 commits into from Oct 26, 2020
Merged

Conversation

DXCanas
Copy link
Member

@DXCanas DXCanas commented Oct 6, 2020

Summary

Was surprised to find that sphinx parsed and converted Markdown, too!

Can leave as is or begin converting to rst, but structure will likely remain the same.

Reviewer guidance

make docs, then check out the build HTML under "Build System"

References

resolves #7089


Contributor Checklist

PR process:

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Testing:

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

Reviewer Checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@DXCanas DXCanas changed the base branch from release-v0.14.x to develop October 6, 2020 21:48
@codecov
Copy link

codecov bot commented Oct 6, 2020

Codecov Report

Merging #7544 into develop will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted Files Coverage Δ
...lity/assets/src/views/FacilityConfigPage/index.vue 59.64% <0.00%> (-2.65%) ⬇️
kolibri/core/assets/src/utils/i18n.js 55.10% <0.00%> (-2.05%) ⬇️
...libri/plugins/user/assets/src/views/SignUpPage.vue 37.63% <0.00%> (-1.95%) ⬇️
kolibri/plugins/coach/assets/src/csv/fields.js 4.22% <0.00%> (-1.34%) ⬇️
...views/onboarding-forms/FacilityPermissionsForm.vue 96.42% <0.00%> (-0.80%) ⬇️
...i/plugins/user/assets/src/views/FacilitySelect.vue 27.27% <0.00%> (-0.73%) ⬇️
...olibri/core/assets/src/mixins/commonCoreStrings.js 33.33% <0.00%> (ø)
...plugins/learn/assets/src/views/RecommendedPage.vue 0.00% <0.00%> (ø)
...ugins/learn/assets/src/views/commonLearnStrings.js 100.00% <0.00%> (ø)
...gins/learn/assets/src/views/RecommendedSubpage.vue 0.00% <0.00%> (ø)
... and 16 more

@indirectlylit
Copy link
Contributor

let me know when you'd like me to review

@DXCanas
Copy link
Member Author

DXCanas commented Oct 7, 2020

let me know when you'd like me to review

I think what I really want to know is if I should bother translating it to RST

@indirectlylit
Copy link
Contributor

If it works (in the sense of appearing correctly in the TOC and having headers etc formatted consistently) I don't think it's necessary

@DXCanas DXCanas marked this pull request as ready for review October 8, 2020 20:36
@DXCanas
Copy link
Member Author

DXCanas commented Oct 8, 2020

Then it's ready!

@radinamatic
Copy link
Member

...surprised to find that sphinx parsed and converted Markdown, too!

Told you it would 😉

I'm not very well versed in the build system engineering, so I cannot really comment on the content of these docs until I manage the time to do more than click the Restart build button on Buildkite ☺️

Just a few comments regarding the writing and structure:

  • Switch to the sentence case for headings to be consistent with the rest of the dev docs
  • Drop the word documentation and rename the H1 to Build system and workflow or similar, for example
  • Move the doc one item below in the main TOC (just under the Development workflow). To me at least, it seems like a more organic place for the build docs.
  • Does this subheading nesting feels OK? Not saying it shouldn't, mostly checking because FAQ on top looks a bit awkward to me...
    Selection_054

No need to go deeper in the language style edits at this point: dev docs are the result of many voices and writing styles that would require a major push to fully edit, revise and update. Just bumped into a phrase with ...For an upcoming 0.3.0 release... 😂

@indirectlylit
Copy link
Contributor

indirectlylit commented Oct 13, 2020

Does this subheading nesting feels OK? Not saying it shouldn't, mostly checking because FAQ on top looks a bit awkward to me...

I think it's fine to lead with the FAQs if they are in fact frequently asked. Might actually spell it out though.

However, the nesting goes too deep. I think the "Architecture" section can be flattened significantly, leaving an outline like:

  • Build system
    • Frequently asked questions
    • Design goals
    • Overview of Buildkite
      • API and concepts
      • Github integration
      • Agents
      • Self-hosting
    • Pipelines
    • Pipeline orchestration
    • Block steps
    • Release builds

@DXCanas
Copy link
Member Author

DXCanas commented Oct 14, 2020

I actually put in the FAQ's based on the issue that spurred the docs:
#7089

Based on the fact that those were put in there, I just assumed they were "frequently asked". I can rename or move the section if y'all think it's necessary 🤷‍♂️

@DXCanas
Copy link
Member Author

DXCanas commented Oct 14, 2020

That nesting might not make the most sense, I think.

Pipelines
Pipeline orchestration
Block steps
Release builds

Shouldn't all be at the same conceptual "level" IMO

Pipelines is describing LE's use of pipelines. Orchestration, block steps, and release builds are all children of that concept; they don't make a lot of sense out of the overarching context of "LE's use of pipelines". That's also why it's named that way. Pipelines themselves were defined in the Buildkite section. We're doing things a little differently, making use of niche features that aren't well documented in Buildkite's docs.

@DXCanas
Copy link
Member Author

DXCanas commented Oct 14, 2020

This is what I went with:
image
image

@radinamatic
Copy link
Member

Looks good to me 💯, but I'll leave the last word to @indirectlylit!

Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

let's do it

@DXCanas
Copy link
Member Author

DXCanas commented Oct 26, 2020

Checking travis errors before merge

@DXCanas DXCanas merged commit defef21 into learningequality:develop Oct 26, 2020
@DXCanas DXCanas deleted the build-pipeline-docs branch October 26, 2020 21:52
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

3 participants