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

clarify readme #56

Merged
merged 26 commits into from
May 9, 2021
Merged

clarify readme #56

merged 26 commits into from
May 9, 2021

Conversation

Bruno-366
Copy link
Contributor

No description provided.

Copy link
Member

@erikmd erikmd left a comment

Choose a reason for hiding this comment

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

Hi @Bruno-366, thanks for opening this!

I know that your PR is still in Draft mode, but I already left a few comments :)

BTW regarding my review comment w.r.t. the section title with action.yaml, do you think you could also add a comment such as the following somewhere (e.g. just after ## Example?) → feel free to rephrase anyway:

Using a GitHub Action in your GitHub repository amounts to committing a file .github/workflows/your-workflow-name.yml, e.g., .github/workflows/build.yml, containing (among others), a snippet such as:

runs-on: ubuntu-latest  # container actions require GNU/Linux
[…]
      ocaml_version: ${{ matrix.ocaml_version }}

Each field can be customized, see below for the documentation of those specific to the docker-coq-action, or the GitHub Actions official documentation for the standard fields involved in workflows.

@erikmd erikmd added the documentation Improvements or additions to documentation label May 6, 2021
Bruno-366 and others added 6 commits May 6, 2021 10:53
Co-authored-by: Erik Martin-Dorel <[email protected]>
move the "optional" to the headings so that when scrolling/browsing and getting a quick overview, one can see that all the inputs (so far) are optional. If there ever is a required input this would make finding it while scrolling easier too
Question: what is the default to `export`?
@Bruno-366
Copy link
Contributor Author

Bruno-366 commented May 6, 2021

two questions: what is default value for export since its optional, and what language is custom_script (the mustaches)? custom_script expands to a bash script, but its written in something else (given that mustaches aren't part of bash). I suspect yaml.

@Bruno-366
Copy link
Contributor Author

I think the remarks should maybe be moved so that they are before the inputs section, if reading them first makes understanding the inputs easier.

@Bruno-366
Copy link
Contributor Author

Also the opam related information should have its own heading/section.
And maybe include a sentence like:

coq is built on-top of ocaml and so coq projects use ocaml's package manager (opam) to build themselves. This github action supports opam out of the box.

@erikmd
Copy link
Member

erikmd commented May 6, 2021

I think the remarks should maybe be moved so that they are before the inputs section, if reading them first makes understanding the inputs easier.

Indeed, why not.

@erikmd
Copy link
Member

erikmd commented May 6, 2021

Also the opam related information should have its own heading/section.

but there's already #### OPAM (?)

or maybe, you just mean move this section with a higher heading, such as ## OPAM ?

And maybe include a sentence like:

coq is built on-top of ocaml and so coq projects use ocaml's package manager (opam) to build themselves. This github action supports opam out of the box.

👍 good idea; you can commit this as is :) (maybe just capitalizing GitHub Action)

@erikmd
Copy link
Member

erikmd commented May 6, 2021

@Bruno-366

two questions: what is default value for export since its optional,

It's true that the current doc https://github.com/coq-community/docker-coq-action/tree/v1.2.3#export only mentions one example with a single variable, so this can be misleading.

To be more precise:

  • export needs to satisfy the regexp ^[a-zA-Z_][a-zA-Z0-9_ ]*$,
  • it is intended to be a space-separated list of environment variables.
  • If it is not passed to docker-coq-action, it just defaults to '', i.e., no additional variable is exported.

− However note that there's an open feature wish suggesting to change the behavior (a.k.a. needing to make each extra export explicit) to some "automatic, implied export of everything": #54 − but I've not yet decided how to deal with this suggestion (being safe/reliable, backward-compatible, and intuitive…)

and what language is custom_script (the mustaches)?

Good question 😉 → it's regular bash, plus some handcrafted support for specific mustaches expressions, as implemented by this code: https://github.com/coq-community/docker-coq-action/blob/v1.2.3/entrypoint.sh#L140-L153

The overall idea being to provide a GitHub container action (docker-coq-action) that works out-of-the-box with docker-coq / opam,
but also can do anything else if we want (using Docker images of, say Java :), while being customizable in two ways:

  • either "in a Travis or so way", customizing all the parts that are implied in the custom_script (install:, script:, etc.), and keeping the default custom_script as is,
  • or overriding the custom_script in a specific way (so, with a Bash script without relying on mustaches → ignoring then the other fields install:…)

@Bruno-366
Copy link
Contributor Author

Also the opam related information should have its own heading/section.

but there's already #### OPAM (?)

or maybe, you just mean move this section with a higher heading, such as ## OPAM ?

And maybe include a sentence like:

coq is built on-top of ocaml and so coq projects use ocaml's package manager (opam) to build themselves. This github action supports opam out of the box.

👍 good idea; you can commit this as is :) (maybe just capitalizing GitHub Action)

I meant there is the information under remarks, and then there is the information at the top. There isn't really a good reason for splitting them up like that, given that both texts are so short, unless the plan is to add substantial more information under remarks.

probably makes it clearer that this github action main use case is probably building coq projects
@Bruno-366 Bruno-366 marked this pull request as ready for review May 7, 2021 11:19
@Bruno-366
Copy link
Contributor Author

just a tip: the table can be added to the other repos and the x on the first column moved to the relevant row.

@erikmd
Copy link
Member

erikmd commented May 9, 2021

I took the opportunity to refine a few other details in the markdown documentation; will squash-merge now.
Thanks again, @Bruno-366 !

@erikmd erikmd merged commit cab4af3 into coq-community:master May 9, 2021
@Bruno-366 Bruno-366 deleted the patch-1 branch May 9, 2021 17:54
erikmd added a commit to coq-community/docker-coq that referenced this pull request May 12, 2021
erikmd added a commit to coq-community/docker-base that referenced this pull request May 12, 2021
@erikmd
Copy link
Member

erikmd commented May 12, 2021

just a tip: the table can be added to the other repos and the x on the first column moved to the relevant row.

FYI @Bruno-366, this is now done :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants