Skip to content

Conversation

@nrichers
Copy link
Collaborator

Internal Notes for Reviewers

Do not commit the output until the next release tag is available

This PR enables cloning the validmind-library at a specific release tag to ensure version-accurate documentation for:

  • Notebooks with code samples
  • Generated Quarto Markdown for our Python API reference
  • Test descriptions generated on the fly
Capto_2025-03-20_07-41-13_am

The detached HEAD state is expected — we are checking out a specific SHA rather than tracking the latest commit (which is HEAD)

Important: It’s safe to run make get-source to test the changes, but do not commit the output yet.

Our docs site now relies on the generated Quarto Markdown for the Python API reference. To avoid breaking the documentation, we must wait for a new release tag in the validmind-library repository that includes this Quarto Markdown before committing the cloned source.

External Release Notes

@nrichers nrichers added infrastructure Docs infra changes internal Not to be externalized in the release notes labels Mar 20, 2025
@validbeck
Copy link
Collaborator

This should be assigned to YOU and reviewed by ME so I've just fixed that, taking a look now!

@validbeck validbeck assigned nrichers and unassigned validbeck Mar 20, 2025
@validbeck validbeck self-requested a review March 20, 2025 21:58
@nrichers nrichers requested a review from nibalizer March 20, 2025 22:12
@nrichers
Copy link
Collaborator Author

@nibalizer added you to this PR for review, mainly for awareness. Related to our convo earlier about cloning repos, as we need to start including only version-accurate files from the validmind-library repo.

(The same is true for the installation repo but that is a follow-on conversation.)

Copy link
Collaborator

@validbeck validbeck left a comment

Choose a reason for hiding this comment

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

  • Just pulled this down and tested it with v2.7.7 — seems to work, as it broke all the API reference .qmd links as outlined!
  • Tried it with v2.0.0 just for kicks too, and yup, bunch o' notebooks missing as expected.

@nrichers
Copy link
Collaborator Author

@validbeck as discussed, I re-enabled cloning from a specific branch as an optional parameter since we both have a use case for this during testing.

The new feature where you clone at a specific release tag works just as before:

❯ make clone
Enter release tag (example: v2.8.10): v2.8.10

Cloning source repo at tag v2.8.10 ...
Cloning into '_source/validmind-library'...
remote: Enumerating objects: 934, done.
remote: Counting objects: 100% (934/934), done.
remote: Compressing objects: 100% (799/799), done.
remote: Total 934 (delta 307), reused 368 (delta 120), pack-reused 0 (from 0)
Receiving objects: 100% (934/934), 50.73 MiB | 21.82 MiB/s, done.
Resolving deltas: 100% (307/307), done.
Note: switching to 'dbf18e618d347904da199982243e69ebba43f332'.

The old optional cloning of a specific branch has been re-enabled, now with a warning:

❯ make clone BRANCH=beck/sc-9082/edit-validation-credit-risk-notebook

WARNING: Cloning non-release files from beck/sc-9082/edit-validation-credit-risk-notebook — DO NOT COMMIT
Cloning into '_source/validmind-library'...
remote: Enumerating objects: 952, done.
remote: Counting objects: 100% (952/952), done.
remote: Compressing objects: 100% (805/805), done.
remote: Total 952 (delta 318), reused 441 (delta 132), pack-reused 0 (from 0)
Receiving objects: 100% (952/952), 51.98 MiB | 25.13 MiB/s, done.
Resolving deltas: 100% (318/318), done.

@nrichers nrichers requested a review from validbeck March 21, 2025 01:08
@github-actions
Copy link
Contributor

PR Summary

This pull request enhances the Makefile used in the project by modifying the clone target. The changes introduce a conditional logic that prompts the user to enter a release tag if the BRANCH environment variable is not set to main. This ensures that the repository is cloned at a specific release tag, which is useful for maintaining consistency and stability in builds. If a branch other than main is specified, a warning is displayed to prevent accidental commits of non-release files. The cloning process now uses shallow cloning (--depth 1) to improve efficiency by reducing the amount of data fetched.

Test Suggestions

  • Test cloning with the BRANCH variable set to 'main' to ensure it skips the tag prompt.
  • Test cloning with the BRANCH variable set to a non-main branch to verify the warning message is displayed.
  • Test cloning without setting the BRANCH variable to ensure the prompt for a release tag appears and functions correctly.
  • Verify that the repository is cloned at the specified tag when a tag is entered.
  • Check that the shallow clone (--depth 1) is performed correctly by inspecting the cloned repository's history.

@github-actions
Copy link
Contributor

A PR preview is available: Preview URL

@validbeck
Copy link
Collaborator

I checked this new edit and it works fine for branches, but what about the usecase where we want to clone from main but NOT at the latest release tag (that might be behind?) ... 🤔

@nrichers
Copy link
Collaborator Author

I checked this new edit and it works fine for branches, but what about the usecase where we want to clone from main but NOT at the latest release tag (that might be behind?) ... 🤔

My take was to be as restrictive as possible while still allowing cloning of some branches, e.g. one you are working on. (which would never be main directly). That said, but eadc353 removes the restriction:

❯ make clone BRANCH=main

WARNING: Cloning non-release files from main — DO NOT COMMIT
Cloning into '_source/validmind-library'...
remote: Enumerating objects: 1158, done.
remote: Counting objects: 100% (1158/1158), done.
remote: Compressing objects: 100% (1025/1025), done.
remote: Total 1158 (delta 346), reused 443 (delta 118), pack-reused 0 (from 0)
Receiving objects: 100% (1158/1158), 51.43 MiB | 24.46 MiB/s, done.
Resolving deltas: 100% (346/346), done.

@github-actions
Copy link
Contributor

A PR preview is available: Preview URL

1 similar comment
@github-actions
Copy link
Contributor

A PR preview is available: Preview URL

@nrichers nrichers force-pushed the nrichers/sc-9266/enable-checking-out-release-tags-for-make branch from 5abad61 to eadc353 Compare March 24, 2025 18:55
@nrichers nrichers force-pushed the nrichers/sc-9266/enable-checking-out-release-tags-for-make branch from eadc353 to 015fa80 Compare March 24, 2025 20:20
@nrichers
Copy link
Collaborator Author

@validbeck as mentioned on our call, my last change broke the script logic for requiring a release tag, with an apology.
015fa80 now allows you to clone the main branch at HEAD so you get the latest files.

Clone at HEAD:

❯ make clone
Enter release tag (example: v2.8.10, or HEAD for latest): HEAD

WARNING: Cloning latest non-release files — DO NOT COMMIT
Cloning into '_source/validmind-library'...
remote: Enumerating objects: 1158, done.
remote: Counting objects: 100% (1158/1158), done.
remote: Compressing objects: 100% (1025/1025), done.
remote: Total 1158 (delta 346), reused 444 (delta 118), pack-reused 0 (from 0)
Receiving objects: 100% (1158/1158), 51.43 MiB | 24.50 MiB/s, done.
Resolving deltas: 100% (346/346), done.

The old syntax still works:

❯ make clone BRANCH=main
Enter release tag (example: v2.8.10, or HEAD for latest): HEAD

WARNING: Cloning latest non-release files — DO NOT COMMIT
Cloning into '_source/validmind-library'...
remote: Enumerating objects: 1158, done.
remote: Counting objects: 100% (1158/1158), done.
remote: Compressing objects: 100% (1025/1025), done.
remote: Total 1158 (delta 346), reused 444 (delta 118), pack-reused 0 (from 0)
Receiving objects: 100% (1158/1158), 51.43 MiB | 25.15 MiB/s, done.
Resolving deltas: 100% (346/346), done.

Cloning PR branches:

❯ make clone BRANCH=beck/sc-9082/edit-validation-credit-risk-notebook

WARNING: Cloning non-release files from beck/sc-9082/edit-validation-credit-risk-notebook — DO NOT COMMIT
Cloning into '_source/validmind-library'...
remote: Enumerating objects: 952, done.
remote: Counting objects: 100% (952/952), done.
remote: Compressing objects: 100% (805/805), done.
remote: Total 952 (delta 318), reused 445 (delta 132), pack-reused 0 (from 0)
Receiving objects: 100% (952/952), 51.98 MiB | 22.14 MiB/s, done.
Resolving deltas: 100% (318/318), done.

Cloning at release tags:

❯ make clone
Enter release tag (example: v2.8.10, or HEAD for latest): v2.8.10

Cloning source repo at tag v2.8.10 ...
Cloning into '_source/validmind-library'...
remote: Enumerating objects: 934, done.
remote: Counting objects: 100% (934/934), done.
remote: Compressing objects: 100% (799/799), done.
remote: Total 934 (delta 307), reused 362 (delta 120), pack-reused 0 (from 0)
Receiving objects: 100% (934/934), 50.73 MiB | 24.61 MiB/s, done.
Resolving deltas: 100% (307/307), done.
Note: switching to 'dbf18e618d347904da199982243e69ebba43f332'.

@nrichers
Copy link
Collaborator Author

nrichers commented Mar 24, 2025

OK, next round ready for review! This PR now integrates Spencer's #677, adding the installation repo cloning piece.

How I tested

1. Clone latest (HEAD)

make clone
# When prompted, enter: HEAD

# Expected and observed:
✓ Prompted for tag
✓ Cloned validmind-library from main when HEAD specified
✓ Showed warning about non-release files
✓ Installation repo cloned from main

2. Clone release tag

make clone
# When prompted, enter: v2.8.10

# Expected and observed:
✓ Prompted for tag
✓ Cloned validmind-library from tag v2.8.10
✓ No warning (since it is a release)
✓ Installation repo cloned from main

3. Clone PR branch

make clone BRANCH=beck/sc-9082/edit-validation-credit-risk-notebook

# Expected and observed:
✓ No prompt (used branch directly)
✓ Cloned validmind-library from specified branch
✓ Showed warning about non-release files
✓ Installation repo cloned from main

4. Clone latest (main + HEAD)

make clone BRANCH=main
# When prompted, enter: HEAD

# Expected and observed:
✓ Prompted for tag
✓ Cloned validmind-library from main when HEAD specified
✓ Showed warning about non-release files
✓ Installation repo cloned from main

All test cases passed successfully, demonstrating that:

  • Both BRANCH and LIBRARY_BRANCH parameters work
  • Appropriate warnings are shown for non-release checkouts
  • Installation repo consistently clones from main
  • Tag prompting works as expected

@github-actions
Copy link
Contributor

A PR preview is available: Preview URL

Copy link
Collaborator

@validbeck validbeck left a comment

Choose a reason for hiding this comment

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

Tested all the new options, and they work as expected! 🎉 Thank you for making the changes.

@nrichers nrichers merged commit 3601100 into main Mar 27, 2025
3 checks passed
@nrichers nrichers deleted the nrichers/sc-9266/enable-checking-out-release-tags-for-make branch March 27, 2025 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure Docs infra changes internal Not to be externalized in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants