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.

Sign up for GitHub

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

Jump to bottom

221.Add force checkout and compile functionality #236

Open
wants to merge 45 commits into
base: main
Choose a base branch
from
Open

221.Add force checkout and compile functionality #236

wants to merge 45 commits into from

Conversation

singhd789
Copy link
Collaborator

@singhd789 singhd789 commented Oct 30, 2024 •
edited by ilaflott
Loading

Describe your changes

This PR adds

  • force-checkout
  • force-compile
  • force-dockerfile

These make it easier to edit scripts in between steps. Previously, if steps were run to make scripts, then re-run, there would just be a print statement that the script existed already. However, if the user wanted to change something, let's say in the checkout script, then re-run to execute it, that functionality was not there. They would've had to edit something in their configuration and start over.

Issue ticket number and link (if applicable)

#221

Checklist before requesting a review

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

@singhd789
Merge branch 'main' of github.com:NOAA-GFDL/fre-cli into 221.force-ch…
751d3a2 
…eckout-and-compile

-also fix decorator function in `run_fremake.py`
@singhd789
#221 Add force_checkout and force_compile arguments
6a13ab5
@singhd789
#221 Merge branch 'main' of github.com:NOAA-GFDL/fre-cli into 221.fo…
9e3037a 
…rce-checkout-and-compile
@singhd789
#221 Merge branch 'main' of github.com:NOAA-GFDL/fre-cli into 221.fo…
6b7e632 
…rce-checkout-and-compile
@singhd789
#221 Update use of click options
a8b5bd5
@singhd789
#221 Merge branch 'main' of github.com:NOAA-GFDL/fre-cli into 221.fo…
b19680f 
…rce-checkout-and-compile
@singhd789
#221 Update click option descriptions and click option functinality
5263a9f
@singhd789
#221 Merge branch 'main' of github.com:NOAA-GFDL/fre-cli into 221.fo…
c8957da 
…rce-checkout-and-compile
@singhd789
#221 Fix click options passed
b3bb118
@singhd789
#221 Update fre make unit test to use null_example
295b04d
@singhd789
#221 Re-create compile script if --force-checkout was used
d947e07
@singhd789
#221 Add -force-dockerfile to re-create dockerfile
10ef4fe
@singhd789
#221 Create functions to remove duplication
65f8a0d
@singhd789
#221 Create function to remove duplication in compile creation
2f31d4f 
- code was repeated if `--force-compile` was used
@singhd789
#221 Create functions to remove duplication
f32a3f6 
- code was repeated if `--force-checkout`, `--force-compile` or `--force-dockerfile` was used
@singhd789
#221 Add --force-dockerfile option
fbce701
@singhd789
#221 Create function to remove duplication
6a99f4d 
- code would be duplicated if `--force-dockerfile` option is used
@singhd789
#221 Fix print statement
a6dd493
@singhd789
#221 Add error statement if container doesn't build
6426b32
@singhd789
#221 Update print statements and change click.echo to print for …
6ae45d5 
…consistency
@singhd789
#221 Accidentally removed --execute options - add back in
8162994
@singhd789
#221 Merge branch 'main' of github.com:NOAA-GFDL/fre-cli into 221.fo…
4bf3d05 
…rce-checkout-and-compile
@singhd789 singhd789 requested a review from rem1776 December 11, 2024 21:52
@singhd789 singhd789 marked this pull request as ready for review January 9, 2025 18:46
@ilaflott
Copy link
Member

Looks fairly well thought out to me here- any tests coming?

maybe add quick desc of what the PR adds in plain english when you can!

@singhd789
Copy link
Collaborator Author

singhd789 commented Jan 10, 2025 •
edited
Loading

Looks fairly well thought out to me here- any tests coming?

maybe add quick desc of what the PR adds in plain english when you can!

Oh wait! I can totally add test to test_run_fremake since that's in there already. (I can also add to the compile test I have going in a PR and work with Avery to add tests for checkout). Maybe the compile test can be merged in first - then I can add to it to test this functionality. Also good idea on the description, I think I meant to add to it when I took it out of draft but then forgot

@singhd789 singhd789 marked this pull request as draft January 10, 2025 21:19
@singhd789
Copy link
Collaborator Author

@ilaflott Thanks for taking a look in your spare time btw

@singhd789 singhd789 marked this pull request as ready for review January 15, 2025 19:34
@ilaflott ilaflott self-requested a review January 17, 2025 22:50
@ilaflott
Copy link
Member

i'd help resolve these conflicts, approve+merge whenever you're ready.

@singhd789
Copy link
Collaborator Author

i'd help resolve these conflicts, approve+merge whenever you're ready.

Thanks @ilaflott. I'll work on merge conflicts and probably ping you when I think everything's good

Dana Singh added 10 commits January 21, 2025 10:54
#221 Merge branch 'main' of github.com:NOAA-GFDL/fre-cli into 221.fo…
83a782a 
…rce-checkout-and-compile

- resolve merge conflicts
#221 Correct camel/snake case
caa86e8 
- these changes will be done in another issue
#221 Re-create combined yaml if force-checkout defined
ce64065
#221 Merge branch 'main' of github.com:NOAA-GFDL/fre-cli into 221.fo…
e19b630 
…rce-checkout-and-compile
#221 Fix variables
bca152f
#221 Fix print statement
5b2d037
#221 Add force options
621ba95
#221 Remove path check - this shouldn't exist anyway in this failure
923d63c
#221 Add tests for force-compile option
a0d0bc4
#221 Use run function directly
6411f2b 
- instead of passing fremake object to run function, just pass compile script path to run
- this allows an already created compile script to also run in parallel (if not being created or if no `force-compile` is used)
@singhd789
Copy link
Collaborator Author

@ilaflott @uramirez8707 @rem1776 Ooook, had to iron out some things that I was doing that I actually didn't want to be doing, but I think this PR should be ok for reviews/tests now 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uramirez8707 uramirez8707 Awaiting requested review from uramirez8707

@rem1776 rem1776 Awaiting requested review from rem1776

@ilaflott ilaflott Awaiting requested review from ilaflott

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@singhd789 @ilaflott