Skip to content

Conversation

caponetto
Copy link

@caponetto caponetto commented Aug 5, 2025

related: #499

This PR updates the CI pipeline for the frontend workspace to fail if there are uncommitted changes in generated code. This helps ensure that all generated files are properly committed and that CI reflects the actual state of the codebase.

Note: The fetch-depth: 0 argument is required to fetch the full Git history, allowing us to retrieve the specific Swagger version referenced by the commit hash in the swagger.version file.

@paulovmr
Copy link

paulovmr commented Aug 5, 2025

/ok-to-test

Copy link

@paulovmr paulovmr left a comment

Choose a reason for hiding this comment

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

/lgtm

@paulovmr
Copy link

paulovmr commented Aug 5, 2025

Hi @thesuperzapper , although this is related to the frontend, because of the directory of the changed file, we might need your approval as well, if you could take a look please. Thanks!

@thesuperzapper thesuperzapper changed the title chore(ws): enforce CI failure on uncommitted generated code changes ci(ws): run client generator on frontend PR check Aug 7, 2025
@thesuperzapper
Copy link
Member

@caponetto @paulovmr @harshad16 @andyatmiami we were discussing this in the meeting today, and came to the concusion that this is probably the best of the options (that is, allowing the frontend devs to explicitly choose which commit of the swagger json to generate from).

This was because:

  1. it ensures that frontend devs don't change the generated files (ci now runs generate and checks for diff)
  2. if we always ran the generator based on the HEAD of the branch, it would result in the next frontend PR having to run the generator (and bring in a lot of diff) to have its PR checks pass

The main question is when and who should be updating the generator commit:

  • OPTION 1: which ever frontend PR needs the new API feature first (or encounters a bug/breaking change)
  • OPTION 2: some regular job which tries to get HEAD to use the latest commit that changed the swagger JSON

Either way, this is an improvement from the current situation, so we can merge.

/lgtm
/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thesuperzapper

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 811a186 into kubeflow:notebooks-v2 Aug 7, 2025
10 checks passed
@github-project-automation github-project-automation bot moved this from Needs Triage to Done in Kubeflow Notebooks Aug 7, 2025
yashpal2104 pushed a commit to yashpal2104/notebooks that referenced this pull request Sep 7, 2025
bhaktinarvekar pushed a commit to bhaktinarvekar/notebooks that referenced this pull request Sep 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants