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.

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

Default command to cqfd run, remove command and automatically init when needed #106

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Ballasi
Copy link

@Ballasi Ballasi commented Oct 30, 2023

As an effort to make cqfd run similarily to known tools, such as sudo, work here has been done to:

  • launch command cqfd run automatically without having to provide the run argument
  • automatically build docker images when necessary, without having to prompt the user into launching cqfd (--)?init
  • have the docker image building phase less intrusive so that, when run via run command, it doesn't spam the user with anything pointless for the user

In order to have cqfd to run similarly to known tools, such as sudo,
the cqfd run command has been removed as the default command is now
"run".

Tests have been updated automatically using the following commands:

 $ sed -i 's/cqfd run/cqfd/g' tests/0*
 $ sed -i -s 's/cqfd(.*) init/cqfd\1 --init/g' tests/0*
 $ sed -i -s 's/cqfd(.*) release/cqfd\1 --release/g' tests/0*
 $ sed -i -s 's/cqfd(.*) flavors/cqfd\1 --flavors/g' tests/0*
 $ sed -i -s 's/cqfd(.*) version/cqfd\1 --version/g' tests/0*

and by manually running the test suite for missing $ cqfd.+ run
commands.
@Ballasi Ballasi force-pushed the sudoisation branch 2 times, most recently from 60591a8 to 99d36ec Compare October 30, 2023 20:19
Thomas Ballasi added 2 commits October 30, 2023 16:22
As the cqfd --init command is going, in the future, to be ran
automatically, work has been done to make it less instrusive.

Rather than showing all of the Docker log, it just shows a throbber
with the current step status, and shows the full log only if the
docker build command failed.
As an effort to make cqfd run similarily to known tools, such as
sudo, the docker image is automatically built if necessary rather
than dying.
@ghost
Copy link

ghost commented Nov 2, 2023

Thank you for this PR -- the proposed changes would certainly be improving the tool.

As discussed, maybe this patch could be split into 2 (or 3) pull request. Notable advantages of this approach would be that:

  • the reviews are easier and more focused;
  • the first two commits could probably be merged more quickly (since auto init and throbber don't seem to be breaking changes). This also means more awesome features to users who would remain on the old cqfd (before removal of cqfq run, which is a breaking change that could block in the review process);
  • the merits of each proposed change can be discussed in isolation (blocking/abandoning one feature doesn't affect the other features).

help Show this help text
--init Initialize project build container
--flavors List flavors from config file to stdout
--release Run argument(s) and release software
Copy link

Choose a reason for hiding this comment

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

Should these 3 now be in the Options: section?

Copy link

Choose a reason for hiding this comment

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

The cqfd_run in the test files' name should probably be updated (maybe only cqfd instead?).

usage
exit 1
"--release")
has_to_release=true
;;
*)
Copy link

Choose a reason for hiding this comment

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

Could be out of scope, but can we support -- signifying the end of the options? Right now, cqfd -- ls doesn't work (shows the help message).

@eroussy
Copy link
Contributor

eroussy commented Nov 3, 2023

That's an interesting proposition to improve the tool, however, I think it is not the direction in which cqfd must evolve.

Particularly, launching cqfd init automatically is a bad idea in my opinion, let me explain.
This command must be launched when the Dockerfile has been changed. There can be two reasons for that :

  • either the user has changed the file manually and has forgotten to call cqfd init
  • or the Dockerfile has been changed without the user's knowledge. It can be a wrong git switch command, or it was modified by another user, whatever.

In both cases, I think it is very important to notify the user that the Dockerfile has been changed. Writing cqfd init command is not long and forces the user to acknowledge the Dockerfile change.

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