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: INFENG-885: add lock-api-state.sh pre-commit check #10030

Closed
wants to merge 55 commits into from

Conversation

davidfluck-hpe
Copy link
Contributor

@davidfluck-hpe davidfluck-hpe commented Oct 8, 2024

Ticket

INFENG-885

Description

Add lock-api-state.sh and lock-published-urls.sh pre-commit hooks to ensure protobuf and auto-generated docs changes get committed, instead of making an additional commit on release branches.

Currently, these scripts were run as part of rph during the release. But because they make modifications and then do commits, we should be doing this as part of the PR process instead of during the release.

lock-api-state.sh regenerates the buf image if there are any protobuf changes, and pre-commit will fail if this is the case. lock-published-urls.sh adds new .rst documents to docs/.redirects/all_published_urls_ever.json. pre-commit will also fail if this happens.

I've also added a new make target for docs: lock-published-urls.

Note: this means that, going forward, if you make any changes to protobuf definitions, you will need to run make -C proto gen-buf-image (from the repo root) to regenerate the buf image as part of your pull request. If you add any docs files, you will also need to run make -C docs lock-published-urls from the repository root to generate the appropriate file and commit it.

I apologize for the messy commit history on this. I know we squash anyway, but I still try not to clutter up the PR history if I can help it. I did a lot of GitHub Action testing.

Test Plan

First, ensure you have pre-commit installed and configured.

Using whichever virtual environment or Python installation you use for Determined, install pre-commit if you haven't already (building Determined at any point should install it). Then, run pre-commit install from the Determined repository. This will install the pre-commit hooks, which should be done anyway so things are properly checked.

lock-api-state

  1. Choose any protobuf definition file under proto/ and make an edit. Adding a comment should suffice.
  2. Add the file and attempt to make a commit. Pre-commit should run and fail because a new buf.image.bin file is generated. git restore both that change and your protobuf definition file change to clean up.
  3. You can also run pre-commit yourself: pre-commit run lock-api-state --all-files should return the same failed result.

lock-published-urls

  1. Add a new .rst file somewhere under docs/. It doesn't need to have anything in it.
  2. Attempt to commit this file. Pre-commit should fail on the lock-published-urls step, while leaving behind a change in docs/.redirects/all_published_urls_ever.json.
  3. You can also run pre-commit yourself: pre-commit run lock-published-urls --all-files. It should fail.
  4. Restore git state as necessary.

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

@cla-bot cla-bot bot added the cla-signed label Oct 8, 2024
Copy link

netlify bot commented Oct 8, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit a033120
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/670d8ab5d9be420008475a9a

Copy link

codecov bot commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.60%. Comparing base (dadf75e) to head (a033120).
Report is 17 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10030      +/-   ##
==========================================
- Coverage   54.60%   54.60%   -0.01%     
==========================================
  Files        1260     1260              
  Lines      157635   157635              
  Branches     3632     3632              
==========================================
- Hits        86082    86073       -9     
- Misses      71419    71428       +9     
  Partials      134      134              
Flag Coverage Δ
backend 45.36% <ø> (-0.02%) ⬇️
harness 72.74% <ø> (ø)
web 54.37% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 5 files with indirect coverage changes

@davidfluck-hpe davidfluck-hpe requested a review from a team as a code owner October 8, 2024 22:05
@davidfluck-hpe davidfluck-hpe requested a review from a team as a code owner October 8, 2024 22:34
@davidfluck-hpe davidfluck-hpe changed the title DRAFT - Do not review - INFNEG-885: add lock-api-state.sh pre-commit check build: DRAFT - Do not review - INFNEG-885: add lock-api-state.sh pre-commit check Oct 8, 2024
@davidfluck-hpe davidfluck-hpe force-pushed the INFENG-885-add-pr-status-checks branch from dccd621 to 4c2add1 Compare October 14, 2024 16:52
tools/scripts. Similar versions of these now exist under pre-commit, but
these specific versions were used by rph, and are no longer necessary,
because we should be running them as PR checks via pre-commit, and not
at the last minute during a release.
pre-commit script a bit.

Clean up lock-published-urls pre-commit script to invoke the new make
target.
@davidfluck-hpe davidfluck-hpe changed the title build: DRAFT - Do not review - INFNEG-885: add lock-api-state.sh pre-commit check INFNEG-885: add lock-api-state.sh pre-commit check Oct 14, 2024
@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

@determined-ci determined-ci requested a review from a team October 14, 2024 21:10
@determined-ci determined-ci added the documentation Improvements or additions to documentation label Oct 14, 2024
@davidfluck-hpe davidfluck-hpe changed the title INFNEG-885: add lock-api-state.sh pre-commit check build: INFNEG-885: add lock-api-state.sh pre-commit check Oct 14, 2024
@davidfluck-hpe davidfluck-hpe changed the title build: INFNEG-885: add lock-api-state.sh pre-commit check build: INFENG-885: add lock-api-state.sh pre-commit check Oct 14, 2024
@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

1 similar comment
@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

Copy link

@molinamelendezj molinamelendezj left a comment

Choose a reason for hiding this comment

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

LGTM from infra side. only one NIT would be if the go version is centralized in the config file, since we have go running in actions and in circle ci just in case the version stay in sync

@davidfluck-hpe
Copy link
Contributor Author

Closing this because I have fundamentally misunderstood a few things about these specific scripts and how they relate to the release process. A big thanks to @stoksc for explaining how they fit in. Stay tuned for possible additional, more-correct changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants