-
-
Notifications
You must be signed in to change notification settings - Fork 237
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
Add GITHUB_AUTH_TOKEN as arg to Dockerfile to RUN commands using phive #2314
Conversation
I think that we should stop maintaining the shell files calling docker and use docker-build github action, like discussed in this thread -> #2256 |
@nvuillam please read the following message carefully so that you understand how this PR makes sense regardless of whether we continue using |
I think I understand that, and I agree, I just would prefer that GITHUB_TOKEN was sent through docker-build standard action, so we could do some same for all other linters also requiring it ^^ |
🦙 MegaLinter status:
|
Descriptor | Linter | Files | Fixed | Errors | Elapsed time |
---|---|---|---|---|---|
✅ BASH | bash-exec | 6 | 0 | 0.01s | |
✅ BASH | shellcheck | 6 | 0 | 0.16s | |
✅ BASH | shfmt | 6 | 0 | 0 | 0.41s |
✅ COPYPASTE | jscpd | yes | no | 3.76s | |
✅ DOCKERFILE | hadolint | 105 | 0 | 12.01s | |
✅ JSON | eslint-plugin-jsonc | 21 | 0 | 0 | 2.7s |
✅ JSON | jsonlint | 19 | 0 | 0.37s | |
✅ JSON | v8r | 21 | 0 | 15.36s | |
markdownlint | 309 | 0 | 231 | 7.48s | |
✅ MARKDOWN | markdown-link-check | 309 | 0 | 6.19s | |
✅ MARKDOWN | markdown-table-formatter | 309 | 0 | 0 | 20.71s |
✅ OPENAPI | spectral | 1 | 0 | 2.12s | |
bandit | 176 | 45 | 2.54s | ||
✅ PYTHON | black | 176 | 0 | 0 | 4.74s |
✅ PYTHON | flake8 | 176 | 0 | 2.17s | |
✅ PYTHON | isort | 176 | 0 | 0 | 1.04s |
✅ PYTHON | mypy | 176 | 0 | 9.16s | |
✅ PYTHON | pylint | 176 | 0 | 14.87s | |
pyright | 176 | 276 | 22.58s | ||
✅ REPOSITORY | checkov | yes | no | 35.75s | |
✅ REPOSITORY | git_diff | yes | no | 0.41s | |
✅ REPOSITORY | secretlint | yes | no | 16.53s | |
✅ REPOSITORY | trivy | yes | no | 31.95s | |
✅ SPELL | cspell | 729 | 0 | 26.31s | |
✅ SPELL | misspell | 550 | 0 | 0 | 0.99s |
✅ XML | xmllint | 3 | 0 | 0 | 0.46s |
✅ YAML | prettier | 81 | 0 | 0 | 3.55s |
✅ YAML | v8r | 23 | 0 | 72.28s | |
✅ YAML | yamllint | 82 | 0 | 1.26s |
See detailed report in MegaLinter reports
@nvuillam as I say, to solve the problem that this PR pretends to solve it is not relevant if it is done with the bash script or with the action that you mention... To get to the point that you comment it is necessary to rewrite all the logic of That must be done with time.... Besides that it is going to require that at least 2 people review it because of the regressions that it can have. I don't know if @echoix feels up to the task that would benefit us in #2273. |
(Read the following as a code review, not bashing 😜) First of all, I think that the variable should be "GITHUB_TOKEN", as that's what I think you're using to authenticate. It'll be easier to follow along knowing that GITHUB_AUTH_TOKEN is in reality GITHUB_TOKEN, where there is more help on the web. Then, here is some docs https://docs.docker.com/build/ci/github-actions/examples/#secrets and https://docs.docker.com/engine/reference/commandline/buildx_build/#secret I need to go, without finishing my message, but it's a good start |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #2314 +/- ##
==========================================
+ Coverage 82.88% 82.90% +0.02%
==========================================
Files 171 171
Lines 4523 4523
==========================================
+ Hits 3749 3750 +1
+ Misses 774 773 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
🦙 MegaLinter status:
|
Descriptor | Linter | Files | Fixed | Errors | Elapsed time |
---|---|---|---|---|---|
✅ BASH | bash-exec | 6 | 0 | 0.01s | |
✅ BASH | shellcheck | 6 | 0 | 0.15s | |
✅ BASH | shfmt | 6 | 0 | 0 | 0.04s |
✅ COPYPASTE | jscpd | yes | no | 3.0s | |
✅ DOCKERFILE | hadolint | 105 | 0 | 8.26s | |
✅ JSON | eslint-plugin-jsonc | 21 | 0 | 0 | 1.82s |
✅ JSON | jsonlint | 19 | 0 | 0.24s | |
✅ JSON | npm-package-json-lint | yes | no | 0.68s | |
✅ JSON | v8r | 21 | 0 | 11.8s | |
markdownlint | 309 | 2 | 231 | 5.96s | |
✅ MARKDOWN | markdown-link-check | 309 | 0 | 5.38s | |
✅ MARKDOWN | markdown-table-formatter | 309 | 2 | 0 | 16.76s |
✅ OPENAPI | spectral | 1 | 0 | 1.67s | |
bandit | 176 | 45 | 1.94s | ||
✅ PYTHON | black | 176 | 0 | 0 | 3.08s |
✅ PYTHON | flake8 | 176 | 0 | 1.73s | |
✅ PYTHON | isort | 176 | 0 | 0 | 0.43s |
✅ PYTHON | mypy | 176 | 0 | 7.06s | |
✅ PYTHON | pylint | 176 | 0 | 11.07s | |
pyright | 176 | 274 | 17.19s | ||
✅ REPOSITORY | checkov | yes | no | 27.53s | |
devskim | yes | 61 | 1.24s | ||
✅ REPOSITORY | dustilock | yes | no | 1.79s | |
✅ REPOSITORY | git_diff | yes | no | 0.03s | |
✅ REPOSITORY | secretlint | yes | no | 8.16s | |
✅ REPOSITORY | syft | yes | no | 0.91s | |
✅ REPOSITORY | trivy | yes | no | 16.58s | |
✅ SPELL | cspell | 729 | 0 | 17.67s | |
✅ SPELL | misspell | 550 | 2 | 0 | 0.55s |
✅ XML | xmllint | 3 | 0 | 0 | 0.03s |
✅ YAML | prettier | 81 | 0 | 0 | 2.52s |
✅ YAML | v8r | 23 | 0 | 53.68s | |
✅ YAML | yamllint | 82 | 0 | 1.17s |
See detailed report in MegaLinter reports
You could have same capabilities but better runtime performances if you request a new MegaLinter flavor.
@bdovaz See how they add it in Super-Linter : https://github.com/github/super-linter/blob/13e5f8b29a34bbf7ce2476292477fa385bec2578/scripts/install-phive.sh#L6-L15 |
@echoix about the variable name, it is not a mistake, it is called that way in PHIVE: And about the rest, you are right I understand that it would serve with a:
That's what you mean, isn't it? But it is true that for that both this PR and #2299 depend on rewriting all workflows to remove the bash script from |
I don't think that using docker action is so complicated to migrate (because it basically does the same than what we do, and our bash scripts are SuperLinter ones, and they migrated ), So I can probably try to do that next week ^^ |
I don't think that only by using docker build action the GITHUB_TOKEN would be used everywhere for every call inside the build environnement (building the containers), we will still need to add them in the generated Dockerfiles |
@bdovaz ok, if PHIVE needs it called that way, fine, but everywhere else that value is called GITHUB_TOKEN, and the value that we will pass it will be called GITHUB_TOKEN, including what GitHub will give us. |
7f6f0ef
to
7bd3c3a
Compare
Context: #2312 (comment)