fix: add content build run --force flag #630

lucasrod16 · 2024-12-19T22:12:28Z

Intent

Provide a way to recover from already a build running errors without having to manually edit the JSON state file

Fixes #603 #620 https://github.com/rstudio/connect/issues/29298

Type of Change

Bug Fix
New Feature
Breaking Change

Approach

add a --force flag to the content build run command. this ignores rsconnect_build_running being set to true in the state file to allow users to recover from already a build running errors without having to manually edit the state file
remove the rsconnect_build_running check from the content build [add,remove] commands
--force will only build NEEDS_ BUILD content by default, but can be combined with --retry to build content in other states
update deprecated logger.warn usage to logger.warning
- https://docs.python.org/3/library/logging.html#logging.Logger.warning
Note
There is an obsolete method warn which is functionally identical to warning. As warn is deprecated, please do not use it use warning instead.

Automated Tests

added tests for --force and already a build running conditions

Directions for Reviewers

Checklist

I have updated CHANGELOG.md to cover notable changes.
I have updated all related GitHub issues to reflect their current state.

github-actions · 2024-12-19T22:16:44Z

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines	Covered	Coverage	Threshold	Status
4822	3581	74%	0%	🟢

New Files

No new covered files...

Modified Files

File	Coverage	Status
rsconnect/actions_content.py	66%	🟢
rsconnect/main.py	62%	🟢
rsconnect/utils_package.py	77%	🟢
TOTAL	68%	🟢

updated for commit: cbbd895 by action🐍

Signed-off-by: Lucas Rodriguez <lucas.rodriguez@posit.co>

aronatkins

Some suggestions; take if you feel they're worthwhile.

aronatkins · 2024-12-20T14:53:46Z

rsconnect/actions_content.py

-        raise RSConnectException("There is already a build running on this server: %s" % connect_server.url)
+    if build_store.get_build_running() and not force:
+        raise RSConnectException(
+            "There is already a build running on this server: %s. "


Should we update this message? This language is misleading/ambiguous.

The content build command is targeting a named server, but that might not be this server (the host running the content build script).

The previous content build command may have exited abnormally.

Possible wording:

A "content build" operation targeting SERVER-URL is still running, or exited abnormally.
Use the '--force' option to override this check.

I like it! Your suggested wording is more clear and accurate. Updated to use suggested wording

aronatkins · 2024-12-20T14:54:35Z

rsconnect/actions_content.py

+    # prompt the user to confirm that they want to --force a build.
+    if force:
+        logger.warning("Please ensure a build is not already running in another terminal before proceeding.")
+        user_input = input("Proceed with the build? Type 'yes' to confirm, any other key to cancel: ").strip().lower()


If the user has given --force, I'm OK without prompting for confirmation. They've already given a command-line option, which is enough intervention.

Agreed. It does feel odd to prompt after providing a --force flag

Updated to remove the prompt

aronatkins · 2024-12-20T14:56:43Z

rsconnect/actions_content.py

@@ -154,7 +157,8 @@ def build_start(
        build_add_content(connect_server, all_content)
    else:
        # --retry is shorthand for --aborted --error --running
-        if retry:
+        # --force has the same behavior as --retry and also ignores when rsconnect_build_running=true


Is there content that is not included in aborted, error, and running? What about NEEDS_BUILD?

Maybe --force should do its best to continue the previous run and not run things that already completed.

Replying to my own comment: Any items that have NEEDS_BUILD are automatically included. Based on that, how do you feel about not making --force act like --retry? Yes, folks can combine --force and --retry, but by default, --force runs only the items that have not yet been run.

Yep, NEEDS_BUILD content is automatically included in the build. My thinking was that folks who end up in situations where the --force flag is needed likely are in a similar state that also requires using --retry, but I have no evidence to back that up and is yet to be seen. I think --force only building NEEDS_BUILD content is a sensible default. Updated --force to only build NEEDS_BUILD content

Signed-off-by: Lucas Rodriguez <lucas.rodriguez@posit.co>

aronatkins · 2024-12-20T19:02:31Z

README.md

@@ -885,6 +885,8 @@ build all "tracked" content that has the status `NEEDS_BUILD`.

 > To re-run failed builds, use `rsconnect content build run --retry`. This will build
 all tracked content in any of the following states: `[NEEDS_BUILD, ABORTED, ERROR, RUNNING]`.
+>
+> If you encounter an error stating that a build operation is already running, you can use `rsconnect content build run --force`. This will override this check and build any content marked as `NEEDS_BUILD`.


Should this include something like, "make sure any previously launched content build operation is no longer active"?

Ahh yeah that's a good suggestion. Added a note for that and made an attempt to improve the flow of the wording as well

Also, improve overall flow of --force docs in readme Signed-off-by: Lucas Rodriguez <lucas.rodriguez@posit.co>

lucasrod16 force-pushed the lucas-content-build-run-force branch from 873bd06 to 38a374d Compare December 19, 2024 22:15

lucasrod16 force-pushed the lucas-content-build-run-force branch 2 times, most recently from ad7de8b to c817f8f Compare December 20, 2024 04:57

fix: add content build run --force flag

8e5071e

Signed-off-by: Lucas Rodriguez <lucas.rodriguez@posit.co>

lucasrod16 force-pushed the lucas-content-build-run-force branch from c817f8f to 8e5071e Compare December 20, 2024 05:00

lucasrod16 marked this pull request as ready for review December 20, 2024 05:12

lucasrod16 requested review from aronatkins and dbkegley December 20, 2024 05:12

aronatkins approved these changes Dec 20, 2024

View reviewed changes

lucasrod16 added 7 commits December 20, 2024 10:33

update error message wording

7319978

Signed-off-by: Lucas Rodriguez <lucas.rodriguez@posit.co>

do not prompt user for confirmation when force provided

e440ee6

Signed-off-by: Lucas Rodriguez <lucas.rodriguez@posit.co>

--force only builds NEEDS_BUILD content by default

89a41c2

Signed-off-by: Lucas Rodriguez <lucas.rodriguez@posit.co>

Merge branch 'main' into lucas-content-build-run-force

43031a0

remove unused import

3161aaf

Signed-off-by: Lucas Rodriguez <lucas.rodriguez@posit.co>

update --force option description

3b832b8

Signed-off-by: Lucas Rodriguez <lucas.rodriguez@posit.co>

document --force option in readme

68575fd

Signed-off-by: Lucas Rodriguez <lucas.rodriguez@posit.co>

aronatkins approved these changes Dec 20, 2024

View reviewed changes

Add note about ensuring no active build process before using --force

cbbd895

Also, improve overall flow of --force docs in readme Signed-off-by: Lucas Rodriguez <lucas.rodriguez@posit.co>

lucasrod16 requested a review from aronatkins December 20, 2024 19:51

aronatkins approved these changes Dec 20, 2024

View reviewed changes

lucasrod16 merged commit 9478983 into main Dec 20, 2024
14 checks passed

lucasrod16 deleted the lucas-content-build-run-force branch December 20, 2024 20:05

lucasrod16 mentioned this pull request Dec 20, 2024

Add --force flag for build process #620

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add content build run --force flag #630

fix: add content build run --force flag #630

lucasrod16 commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading

aronatkins left a comment

aronatkins Dec 20, 2024

lucasrod16 Dec 20, 2024

aronatkins Dec 20, 2024

lucasrod16 Dec 20, 2024

lucasrod16 Dec 20, 2024

aronatkins Dec 20, 2024

aronatkins Dec 20, 2024

lucasrod16 Dec 20, 2024

aronatkins Dec 20, 2024

lucasrod16 Dec 20, 2024

fix: add content build run --force flag #630

fix: add content build run --force flag #630

Conversation

lucasrod16 commented Dec 19, 2024 • edited Loading

Intent

Type of Change

Approach

Automated Tests

Directions for Reviewers

Checklist

github-actions bot commented Dec 19, 2024 • edited Loading

☂️ Python Coverage

Overall Coverage

New Files

Modified Files

aronatkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasrod16 commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading