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

protobuf: automate derived file generation #6136

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Jun 13, 2024

(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):

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 oliver-sanders added this to the 8.4.0 milestone Jun 13, 2024
@oliver-sanders oliver-sanders self-assigned this Jun 13, 2024
@oliver-sanders
Copy link
Member Author

(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
Copy link
Member Author

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.

@oliver-sanders oliver-sanders force-pushed the protobuf branch 2 times, most recently from 54b4af6 to d72859b Compare June 21, 2024 12:00
* 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.
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Had a quick look at this so far

Comment on lines +25 to +32
- 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
Copy link
Member

Choose a reason for hiding this comment

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

Is there any point using an environment?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@oliver-sanders oliver-sanders Aug 9, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@oliver-sanders oliver-sanders Aug 9, 2024

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

(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" 😆 image

Copy link
Member Author

@oliver-sanders oliver-sanders Aug 9, 2024

Choose a reason for hiding this comment

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

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 MetRonnie self-requested a review August 6, 2024 16:44
Comment on lines +73 to +74
if [[ -z $(git diff --stat) ]]; then
echo '::error:: No functional changes made to the protobuf schema'
Copy link
Member

Choose a reason for hiding this comment

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

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'

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

Choose a reason for hiding this comment

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

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

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 }}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@oliver-sanders
Copy link
Member Author

Drafting in response to #6136 (comment)

@oliver-sanders oliver-sanders marked this pull request as draft September 30, 2024 15:53
@oliver-sanders oliver-sanders modified the milestones: 8.4.0, some-day Oct 7, 2024
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.

2 participants