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

feat: Add CLI option learn-ocaml build --build-dir=[./_learn-ocaml-build] #585

Merged
merged 3 commits into from
Feb 12, 2024

Conversation

erikmd
Copy link
Member

@erikmd erikmd commented Feb 12, 2024

(…where exercises are precompiled).

Cc @AltGr FYI:

  • I successfully tested the new CLI option on my machine;
  • I am currently building the Docker image to test the new entrypoint as well;
  • Given the patch is relatively straightforward (in particular, rsync did not seem required),
    if you don't object now, I plan to merge it as soon as the CI is green and my local Docker tests pass as well,
    then prepare the release notes (just reordering the CHANGES items in chore(master): release 1.0.0 #572),
    and then pass the baton to you! for merging the release-please PR and taking care of the release announcement.

Kind regards.

Description

  • Kind: enhancement

Motivation:

Examples:

  • learn-ocaml build --repo=demo-repository
  • learn-ocaml build --repo=demo-repository serve
  • learn-ocaml build --repo=demo-repository serve --replace # or
  • learn-ocaml build --repo=demo-repository --build-dir=demo-repository # to retrieve the previous behavior.

Checklist

Note to maintainers

  • Read this wiki page.
  • Make sure the PR has a milestone.
  • Assign yourself before merging.
  • Either do a regular merge:
    • for PRs containing several commits following conventional-commits,
    • or for PRs containing 1 commit shared with a later PR (to preserve the SHA1)
  • Or do a squash-merge:
    • for PRs containing only 1 commit (not shared with a later PR),
    • or for PRs containing several commits that need not be kept in the history;
    • Update the commit message header with a conventional-commit type,
    • Add a footer Close #… if a related issue exists.

…ild]` where exercises are precompiled

Motivation:
- in current master, the exercises were always precompiled in-place;
- this conflicted with the documented workflow (option "ro"):
  https://ocaml-sf.org/learn-ocaml/howto-deploy-a-learn-ocaml-instance.html
  https://ocaml-sf.org/learn-ocaml/howto-setup-exercise-development-environment.html
- this new option makes it possible to copy and precompile exercises apart by default
  (using the learn-ocaml binary, or the official Docker image).

Examples:
- learn-ocaml build --repo=demo-repository
- learn-ocaml build --repo=demo-repository serve
- learn-ocaml build --repo=demo-repository serve --replace
  # or
- learn-ocaml build --repo=demo-repository --build-dir=demo-repository
  # to retrieve the previous behavior.
@erikmd erikmd self-assigned this Feb 12, 2024
@erikmd erikmd added the kind: enhancement Enhancement to an existing user-facing feature. label Feb 12, 2024
@erikmd
Copy link
Member Author

erikmd commented Feb 12, 2024

Hi @AltGr ! Good news, the feature is OK.
However after extensive testing, I noted the following:

  • externalizing the build-dir in a volume looks unneeded
  • indeed, if the build-dir is shared, but the container is deleted, the www is deleted as well, and the exercices are rebuilt anyway
  • moreover, adding another volume adds more noise - more work needed regarding docker run -v args

I need to be away from keyboard for ~2h but I'll push another commit to fix this afternoon (and squash the PR in the end).

Meanwhile, to be on the safe side, I'll tweak another detail in this PR (if the build-dir is "learn-ocaml-build" or "./learn-ocaml-build", then the learn-ocaml-build/exercises subfolder is "rm -fr" to be sure that there is no obsolete leftover (I know this is a bit aggressive, but let's see this as a poor man's "rsync --delete-delay"))


Finally, I notice that the "--replace" feature could be improved regarding Docker side, but this will deserve another PR (after 1.0.0)

- as externalizing the build-dir in a volume looks unneeded:
  if the build-dir is shared, but the container is deleted,
  the www is deleted as well, and the exercices are rebuilt anyway

- and as an extra volume would add more noise (mandating "-v args" tweaks)
@erikmd erikmd force-pushed the learn-ocaml-build-dir branch 2 times, most recently from e01fe4b to 2ac4824 Compare February 12, 2024 18:16
@erikmd erikmd merged commit 6535692 into ocaml-sf:master Feb 12, 2024
12 checks passed
@erikmd erikmd deleted the learn-ocaml-build-dir branch February 12, 2024 19:29
@erikmd erikmd changed the title feat: Add CLI option learn-ocaml build --build-dir=[./learn-ocaml-build] feat: Add CLI option learn-ocaml build --build-dir=[./_learn-ocaml-build] Feb 12, 2024
@erikmd erikmd added this to the learn-ocaml 1.0.0 milestone Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Enhancement to an existing user-facing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant