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

docs: Add four Sections in README.md + one demo GHA job #38

Merged
merged 12 commits into from
Aug 11, 2021
Merged

Conversation

erikmd
Copy link
Member

@erikmd erikmd commented Dec 6, 2020

@Zimmi48 this is not a urgent PR (only about docs), but you may want to proofread it before we can merge it

@erikmd erikmd added the documentation Improvements or additions to documentation label Dec 6, 2020
@erikmd erikmd requested a review from Zimmi48 December 6, 2020 17:07
@Zimmi48
Copy link
Member

Zimmi48 commented Dec 7, 2020

I'm not quite sure what the use case is. Is this about leaking secret variables?

@erikmd
Copy link
Member Author

erikmd commented Dec 15, 2020

I'm not quite sure what the use case is. Is this about leaking secret variables?

Yes, the use case to "hide" some commands from the log would not very useful by itself, indeed, but the fact the variables' value is displayed is a side-effect of the set -x facility, just run:

bash -c 'export foo="42";
  set -x;
  bar="$foo"'

Here, $foo is expanded during the informative log. This is informative, indeed, but may leak secret variables if an user happens to do a curl-or-so deployment within the docker-coq script.

While it would also be possible for the users, and maybe more standard, to do a deployment in a later stage from a dedicated action, given that FYI, the sources files (and the whole workspace of the project that can be modified within the docker-coq-action container) is kept. Only the installed opam package is not available in the end, in particular because we run opam uninstall in the end (but this is not the only reason: the .opam folder itself is lost anyway).

So what's your opinion about this documentation PR?

At least, it seems to me that this limitation has to be documented. This way or another way?
unless you'd just be against set -x henceforth...

@Zimmi48
Copy link
Member

Zimmi48 commented Dec 15, 2020

It seems a good idea to document it, but I would make it even more explicit (giving explicitly the example of not leaking secret variables).

@erikmd
Copy link
Member Author

erikmd commented Aug 10, 2021

Hi @Zimmi48, FYI the potential issue is not critical at all, given repository secrets ${{ secrets.STH }} are automatically hidden, and some secrets that would be obtained by other means can also be marked as hidden in a previous run: step.

FTR, I've tested this again in this now-merged PR: erikmd/docker-coq-github-action-demo#12

Nevertheless, I'll rework this PR #38 soon so that we can integrate that non-critical-albeit-useful documentation.

@erikmd erikmd marked this pull request as draft August 10, 2021 18:51
@erikmd erikmd force-pushed the docs-verbose branch 4 times, most recently from 187bfd7 to 6f4270c Compare August 11, 2021 10:42
@erikmd erikmd marked this pull request as ready for review August 11, 2021 12:52
@erikmd erikmd changed the title Document the use of set -x docs: Add four Sections in README.md + one demo GHA job Aug 11, 2021
@erikmd
Copy link
Member Author

erikmd commented Aug 11, 2021

Hi @Zimmi48, done: I've just finished my documentation rework 🙂

I can propose to do a point release v1.2.4 as soon as PRs #38 and #61 are merged (maybe tonight or tomorrow?)

Feel free to take a look at this new documentation if you want
(as experience tells that "documentation proofread" is always useful :)
but no worries if you can't do it right now and prefer that I merge the two PRs as is.

To sum up, I've added:

and in PR #61:

  • A section References with a link to the "self-documenting tests" from docker-coq-action's CI
  • A section Versioning

@erikmd erikmd self-assigned this Aug 11, 2021
@Zimmi48
Copy link
Member

Zimmi48 commented Aug 11, 2021

Thanks a lot @erikmd! I will have a look today.

Copy link
Member

@Zimmi48 Zimmi48 left a comment

Choose a reason for hiding this comment

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

I will come back and review the last section and the other PR later today.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@erikmd

This comment has been minimized.

@Zimmi48

This comment has been minimized.

Copy link
Member

@Zimmi48 Zimmi48 left a comment

Choose a reason for hiding this comment

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

LGTM

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@erikmd erikmd merged commit 953471f into master Aug 11, 2021
@erikmd erikmd deleted the docs-verbose branch August 11, 2021 17:33
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