protobuf: automate derived file generation #6136

oliver-sanders · 2024-06-13T15:37:01Z

(Training day washup)

Sibling: cylc/cylc-uiserver#606

Take the guesswork out of protobuf schema changes by automating the updating of the derived Python files:

Use the bufbuild/buf tool to:
- Lint the protobuf schema.
- Check for breaking changes.
- Configure the generation of derived Python files.
Address lint failures:
- Ignore the capitalisation of the URL field (protobuf likes snake case).
- Define the schema's "package".
- Define the schema's version (made this match the Cylc API version number).
- Locate the schema in a directory hierarchy which matches "/" (some protobuf standard).
Add a GitHub action which:
- Lints & checks for breaking changes.
- Automates the checking and generation of derived files.
- Commits the results back on pull request branches.

Example CI runs (my fork):

Adding a new field (files updated and committed back to branch)
Removing a field (breaking change)

Note:

Requires a small cylc-uiserver change to match.

Check List

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
Tests are included (or explain why tests are not needed).
CHANGES.md entry included if this is a change that can affect users
Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

oliver-sanders · 2024-06-13T15:41:06Z

(note the protobuf compatibility test will correctly fail on this branch because the schema file it is comparing against does not exist on the branch it is being merged into)

oliver-sanders · 2024-06-19T14:34:23Z

Moving the protobuf files in with the network code introduced circular import issues due to the definition of code in cylc.flow.network.__init__.py. Moved this code into its own modules to resolve the issue.

* Use the bufbuild/buf tool to: * Lint the protobuf schema. * Check for breaking changes. * Configure the generation of derived Python files. * Adds a GitHub action which automates the checking and generation of derived files and commits the results back on pull request branches.

* Some code in the __init__ file was creating a circular dependency issue. * Move this code into its own modules, this may have efficiecncy benefits for other network modules when the code in __init__ is not required.

MetRonnie

Had a quick look at this so far

MetRonnie · 2024-08-06T16:14:19Z

.github/workflows/protobuf.yml

+      - name: Install Protobuf
+        uses: mamba-org/setup-micromamba@v1
+        with:
+          # install protobuf into a mamba env (note use shell = "bash -el {0}"
+          # to access this envionment)
+          environment-name: protobuf
+          create-args: protobuf
+          init-shell: bash


Is there any point using an environment?

Conda/Mamba can only install packages into isolated environments, they cannot install into the system environment.

Why do we need an environment on the temporary GH Actions runner?

You cannot install software using mamba/conda without using an environment. It is not possible.

I am asking why are you using mamba/conda instead of pip (which is simpler - no bash -el {0} needed)?

Mamba installs low-level dependencies properly, pip doesn't.

I think I had issues installing from pip/npm but it was a while back so I can't quite remember the deal.

MetRonnie · 2024-08-06T16:15:59Z

.github/workflows/protobuf.yml

+          # lint .proto files
+          eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)"  # activate homebrew
+          cd cylc/flow/network/protobuf
+          buf lint


(Looks like buf can be installed via npm which might be easier to use in GH Actions, rather than activating homebrew every step)

Btw, first result when I googled "buf" 😆

I wouldn't have used a system package manager if I could have avoided it (I don't like system package managers), but I can't remember. Possibly to ensure that dependencies (incl Node) are installed correctly?

MetRonnie · 2024-08-09T10:24:47Z

.github/workflows/protobuf.yml

+          if [[ -z $(git diff --stat) ]]; then
+            echo '::error:: No functional changes made to the protobuf schema'


Suggested change

if [[ -z $(git diff --stat) ]]; then

echo '::error:: No functional changes made to the protobuf schema'

if git diff --exit-code --stat; then

echo '::warning::No functional changes made to the protobuf schema'

MetRonnie · 2024-08-09T10:27:47Z

.github/workflows/protobuf.yml

+            echo '::error:: No functional changes made to the protobuf schema'
+            exit 0
+          else
+            echo '::info:: pushing update commit'


Suggested change

echo '::info:: pushing update commit'

echo '::notice::pushing update commit'

MetRonnie · 2024-08-09T10:36:29Z

.github/workflows/protobuf.yml

+              git add -u
+              git commit -m 'protobuf: updating generated files'
+              git remote add pr https://github.com/${{ github.event.pull_request.head.repo.owner.login }}/cylc-flow
+              git push pr HEAD:${{ github.head_ref }}


Ah, I'm not sure this will work for a PR from a fork ☹️

I think GITHUB_TOKEN only has read permissions in PRs from forks.

Dammit (although the workflow would work on their fork).

Only one way to find out. My master branch is still @ f3342c2, maybe try raising a PR there (along the lines of the one in the OP) and see what happens?

I know there are other repos that manage to do this. E.G. checkout jupyter-server#1448 (deliberately not linking their issue), those pre-commit commits are pushed onto the branch automatically.

https://github.com/jupyter-server/jupyter_server/pull/1448

I think that is done by an app rather than GH Actions: https://github.com/marketplace/pre-commit-ci

Didn't work ☹️ oliver-sanders#78 (comment)

oliver-sanders added this to the 8.4.0 milestone Jun 13, 2024

oliver-sanders self-assigned this Jun 13, 2024

oliver-sanders force-pushed the protobuf branch from b12e58c to 6bb3301 Compare June 19, 2024 14:31

oliver-sanders mentioned this pull request Jun 19, 2024

adapt to cylc-flow code refactor cylc/cylc-uiserver#606

Open

oliver-sanders force-pushed the protobuf branch 2 times, most recently from 54b4af6 to d72859b Compare June 21, 2024 12:00

oliver-sanders added 2 commits June 21, 2024 13:29

network: move code out of __init__file

c908539

* Some code in the __init__ file was creating a circular dependency issue. * Move this code into its own modules, this may have efficiecncy benefits for other network modules when the code in __init__ is not required.

oliver-sanders force-pushed the protobuf branch from d72859b to c908539 Compare June 21, 2024 12:30

oliver-sanders requested a review from dwsutherland July 1, 2024 15:14

oliver-sanders requested a review from MetRonnie July 12, 2024 09:49

MetRonnie reviewed Aug 6, 2024

View reviewed changes

MetRonnie self-requested a review August 6, 2024 16:44

MetRonnie reviewed Aug 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

protobuf: automate derived file generation #6136

protobuf: automate derived file generation #6136

oliver-sanders commented Jun 13, 2024 •

edited

Loading

oliver-sanders commented Jun 13, 2024

oliver-sanders commented Jun 19, 2024

MetRonnie left a comment

MetRonnie Aug 6, 2024

oliver-sanders Aug 7, 2024

MetRonnie Aug 9, 2024

oliver-sanders Aug 9, 2024 •

edited

Loading

MetRonnie Aug 9, 2024

oliver-sanders Aug 9, 2024 •

edited

Loading

MetRonnie Aug 6, 2024

oliver-sanders Aug 9, 2024 •

edited

Loading

MetRonnie Aug 9, 2024

MetRonnie Aug 9, 2024

MetRonnie Aug 9, 2024

oliver-sanders Aug 9, 2024

MetRonnie Aug 9, 2024

MetRonnie Aug 9, 2024

MetRonnie Aug 9, 2024

		if [[ -z $(git diff --stat) ]]; then
		echo '::error:: No functional changes made to the protobuf schema'

	echo '::info:: pushing update commit'
	echo '::notice::pushing update commit'

protobuf: automate derived file generation #6136

Are you sure you want to change the base?

protobuf: automate derived file generation #6136

Conversation

oliver-sanders commented Jun 13, 2024 • edited Loading

oliver-sanders commented Jun 13, 2024

oliver-sanders commented Jun 19, 2024

MetRonnie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders commented Jun 13, 2024 •

edited

Loading

oliver-sanders Aug 9, 2024 •

edited

Loading

oliver-sanders Aug 9, 2024 •

edited

Loading

oliver-sanders Aug 9, 2024 •

edited

Loading