switching checkout repo; fixing bug in branch checkout

cwhitlock-NOAA commented 1 month ago

Describe your changes

Replaced gitlab checkout with github checkout; fixed bug in checkout that hard-coded branch="main"

Issue ticket number and link (if applicable)

Checklist before requesting a review

[ x] I ran my code
[ x] I tried to make my code readable
[ x] I tried to comment my code
[ ] I wrote a new test, if applicable
[ ] I wrote new instructions/documentation, if applicable
[ x] I ran pytest and inspected it's output
[ ] I ran pylint and attempted to implement some of it's feedback

This probably does deserve a pytest test for teh branch checkout, but that's another to-do item

ilaflott commented 1 month ago

i completely agree that fre pp checkout needs a test. This shouldn't be too bad, but i do wonder if github actions will let us do it.

the test would look like one of the other clirunner() tests, and it would check for the cloned repo directory, AND that the directory is non-empty.

def test_pp_checkout():
    result = runner.invoke(fre.fre, args=["pp checkout", otherarg1, otherarg2])
    assert all( [ result.exit_code == 0,
                  pathlib.Path('./fre-workflows').exists(),
                  <some way to make sure dir not empty>
                 ] )

cwhitlock-NOAA commented 1 month ago

Thank you for the review! I know that it's possible to skip all tests when pushing, but skipping an individual one via the git commands is another matter.

I'd also like to check for something more than is-present and is-not-empty

the bugfix is in response to checking out the wrong branch (was hard-coded to "main"). This might be a misuse of the "protected branch" function - but if we kept around a branch that wasn't named "main" in the workflows repo, I can check against that to make sure that the checks are working.

(And the branch-specific checkouts are needed for running the workflow regtests - you need to have the current branch you are referring to referenced by the checkout, not the main branch of the workflows repo. Getting the checkout tests working here is an incremental step towards getting the workflow tests working there.)

On Fri, Jul 26, 2024 at 10:06 AM Ian @.***> wrote:

@.**** requested changes on this pull request.

lets try writing the test for checkout, and see if github will let us run it in CI/CD.

if github doesn't, learning how to skip the test if python notices it's running in a CI/CD is also a good thing, i imagine.

— Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/fre-cli/pull/131#pullrequestreview-2201952259, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC362WQLCPH4PYXLAMULL73ZOJJXBAVCNFSM6AAAAABLP3QYJCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMBRHE2TEMRVHE . You are receiving this because you authored the thread.Message ID: @.***>

-- Carolyn Whitlock

ilaflott commented 1 month ago

> I'd also like to check for something more than is-present and is-not-empty

admirable- but lets grapple with that after we author a basic check! a simple, yet imperfect check, is better than no check!

> the bugfix is in response to checking out the wrong branch (was hard-coded to "main").

what bug? confused!

> This might be a misuse of the "protected branch" function - but if we kept around a branch that wasn't named "main" in the workflows repo, I can check against that to make sure that the checks are working.

still confused. we do expect to have more than one branch for the workflows repo in general FYI.

> And the branch-specific checkouts are needed for running the workflow regtests - you need to have the current branch you are referring to referenced by the checkout, not the main branch of the workflows repo. Getting the checkout tests working here is an incremental step towards getting the workflow tests working there.

let's start with any branch checkout being tested first, merge this code in with that check ASAP if possible, and then we can

ceblanton commented 1 month ago

Completely agree with @ilaflott 's suggestions for increasing test coverage for fre pp checkout.

ceblanton commented 1 month ago

> I'd also like to check for something more than is-present and is-not-empty

admirable- but lets grapple with that after we author a basic check! a simple, yet imperfect check, is better than no check!

> the bugfix is in response to checking out the wrong branch (was hard-coded to "main").

what bug? confused!

> This might be a misuse of the "protected branch" function - but if we kept around a branch that wasn't named "main" in the workflows repo, I can check against that to make sure that the checks are working.

still confused. we do expect to have more than one branch for the workflows repo in general FYI.

> And the branch-specific checkouts are needed for running the workflow regtests - you need to have the current branch you are referring to referenced by the checkout, not the main branch of the workflows repo. Getting the checkout tests working here is an incremental step towards getting the workflow tests working there.

let's start with any branch checkout being tested first, merge this code in with that check ASAP if possible, and then we can

For now, I don't think we should (or can, really) validate which branch fre pp checkout is checking out. Eventually though, we want more safety guardrails to ensure consistent behavior (i.e. compatibility between the fre-cli version and the fre-workflows version).

ceblanton commented 1 month ago

I noted the desired tests in a new issue, #136. I'm going to merge this small update since we need it for fre/2024.01

ceblanton commented 1 month ago

@ilaflott I was going to merge but our rules prevent it! This is probably a good feature.

New tests for fre pp checkout are tracked in #136. Can you approve this PR as is?

NOAA-GFDL / fre-cli