infra: docs-as-code setup#3
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
AlexanderLanin
left a comment
AlexanderLanin
left a comment
There was a problem hiding this comment.
AI-assisted review (Claude):
Critical: 1 | Important: 1 | Suggestions: 1
Progress since last review: The tar pipeline is fixed cleanly — both the broken extraction step and the stale tar creation were removed. The basics push trigger is gone. Two prior findings are now resolved.
Still open:
- Critical —
$TARGETundefined inExtend versions.json(see inline comment). This corruptsversions.jsonon every deployment. - Important — Artifact action version mismatch:
upload-artifact@v7.0.1(pr.yml) vsdownload-artifact@v8.0.1(deploy-pages.yml). Align to the same major version. - Suggestion — No cleanup on PR close.
pr-N/folders and theirversions.jsonentries accumulate forever. Copilot flagged this too. Add a workflow triggered onpull_request: [closed]that removes the folder and the correspondingversions.jsonentry.
MaximilianSoerenPollak
left a comment
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
Looks good overall.
Just some nitpicks / questions though
|
|
||
| - name: Build documentation | ||
| run: | | ||
| bazel run //:docs -- --github_user=${{ github.repository_owner }} --github_repo=${{ github.event.repository.name }} |
There was a problem hiding this comment.
Should enable --lockfile_mode=error here too no, to be consitent with the rest of S-Core?
There was a problem hiding this comment.
Docs should not fail because of lockfiles. Lockfiles should fail because of lockfiles 🔒
But since we dont have proper "buy-in" on https://github.com/eclipse-score/cicd-workflows/blob/main/.github/workflows/on-pr.yml I have excluded it for now.
| # It's just the usual maintainer assignment that we perform per repository. | ||
| # It will be updated as the repository evolves, based on real life experience on who is actively maintaining (not contributing to!!) the repository. | ||
|
|
||
| * @AlexanderLanin @MaximilianSoerenPollak @dcalavrezo-qorix @pawelrutkaq @PiotrKorkus @lurtz @nradakovic @opajonk @antonkri @FScholPer @anmittag |
There was a problem hiding this comment.
I think for now this makes sense.
Might be good for future itterations to have certain people review changes in certain folders that pretend to them.
Like infra for us, or testing for piotr etc.
There was a problem hiding this comment.
I dont want a setup like in score, where it is plain impossible to land a cross topic PR
There was a problem hiding this comment.
That's true.
Though I mean here it would make almost more sense cause it strictly is 'just' documentation.
But ye i agree it makes sense to keep it like this for now.
MaximilianSoerenPollak
left a comment
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
All issues addressed.
Looks alright to me.
No description provided.