Skip to content

ENH: only run diffed content #49

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

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

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented May 13, 2025

There are some problems with this, e.g. I don;t have origin locally, and the logic didn't fully work with github URLs (nor I want to hardwire the repo link in the test file).

So, do we want this to just be done in CI, but locally we test with all the files? I'm not fully certain what's the best approach.

test.sh Outdated
if [ $# -gt 0 ]; then
files_to_process="$@"
if [[ "$1" == all ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if [[ "$1" == all ]]; then
if [[ "$1" == --all ]]; then

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make --all the default because it requires no further input for the user.

test.sh Outdated
else
files_to_process=$all_markdown_files
# We only want to run tests for the notebooks we touched
files_to_process=$(git fetch origin main --depth=1; git diff origin/main --name-only tutorials | grep .md)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the usage is --changed <branch> we can avoid hard-coding a specific branch name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, and the we can hard-code origin and main on CI as those are set up there.

@danielballan danielballan marked this pull request as ready for review May 14, 2025 14:28
@bsipocz
Copy link
Member Author

bsipocz commented May 15, 2025

fyi: I'm stuck with the git logic a bit, to figure out a generic way for the arguments to pass on to git fetch/etc, as opposed to what we had to be run on CI only before.

@bsipocz bsipocz force-pushed the CI_run_only_diffed branch 2 times, most recently from 921f875 to 52287fb Compare May 15, 2025 22:10
@danielballan
Copy link
Contributor

I took this for a spin. Maybe we need two arguments:

remote=$2
branch=$3

so that we can git fetch ... ${2} and then git diff against ${2}/{3}? You might have a different usage in mind, though.

@bsipocz
Copy link
Member Author

bsipocz commented May 16, 2025

Yes, that's the one I converged on yesterday, I think I just sent myself down to unnecessary rabbit holes during the summit of automatically finding base branches for the default case of not providing arguments (e.g. I want the tox file to be convenient both locally and on CI)

@bsipocz
Copy link
Member Author

bsipocz commented May 16, 2025

(FYI: I'm also using this PR to test the redirect mess as it's still not 100% up and working)

@bsipocz bsipocz force-pushed the CI_run_only_diffed branch from 52287fb to 3565257 Compare May 21, 2025 01:33
@bsipocz
Copy link
Member Author

bsipocz commented May 21, 2025

OK, finally the redirector action works. I'm still puzzled why the context conditional didn't work, but leave that rabbit hole for another day.

@danielballan
Copy link
Contributor

Cool.

Testing this locally, I do see it successfully detecting local changes and only testing the changed notebooks. We just need to pull apart branch and remote into separate vars so that git fetch works. Currently:

pixi run test --changed origin/main
...
fatal: 'origin/main' does not appear to be a git repository
fatal: Could not read from remote repository.

@bsipocz bsipocz marked this pull request as draft May 21, 2025 15:37
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