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

Add GITHUB_AUTH_TOKEN as arg to Dockerfile to RUN commands using phive #2314

Merged
merged 5 commits into from
Feb 11, 2023

Conversation

bdovaz
Copy link
Collaborator

@bdovaz bdovaz commented Feb 5, 2023

Context: #2312 (comment)

@bdovaz bdovaz requested a review from nvuillam as a code owner February 5, 2023 21:50
@bdovaz bdovaz mentioned this pull request Feb 5, 2023
@nvuillam
Copy link
Member

nvuillam commented Feb 5, 2023

I think that we should stop maintaining the shell files calling docker and use docker-build github action, like discussed in this thread -> #2256

@bdovaz
Copy link
Collaborator Author

bdovaz commented Feb 5, 2023

@nvuillam please read the following message carefully so that you understand how this PR makes sense regardless of whether we continue using upload-docker.sh or not please.... I have tried in several messages to make you understand but either I don't explain myself well or I don't know :(

#2312 (comment)

@nvuillam
Copy link
Member

nvuillam commented Feb 5, 2023

That is, we have to modify the descriptor to pass that environment variable to each phive command. Example:

Line 133 in b0bb50f

      RUN phive --no-progress install psalm -g --trust-gpg-keys 8A03EA3B385DBAA1,12CE0F1D262429A5

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 ^^

@nvuillam
Copy link
Member

nvuillam commented Feb 5, 2023

🦙 MegaLinter status: ⚠️ WARNING

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
⚠️ MARKDOWN 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
⚠️ PYTHON 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
⚠️ PYTHON 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

MegaLinter is graciously provided by OX Security

@bdovaz
Copy link
Collaborator Author

bdovaz commented Feb 5, 2023

@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 upload-docker.sh to use that action instead of the bash script and that is a titanic task and that requires a lot of precision because that script is very big and has many conditions, etc...

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.

@echoix echoix self-requested a review February 5, 2023 22:32
@echoix
Copy link
Collaborator

echoix commented Feb 5, 2023

(Read the following as a code review, not bashing 😜)
I think that there's a little better way to implement what you have just added, as what I understand, it might appear in the image's history.

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
We can use the secrets feature of docker building. Look also at https://stackoverflow.com/questions/68275321/how-to-use-github-token-in-dockerfile-that-is-built-in-github-actions-and-trying, where in the question his usage is right.
I'm not sure, but maybe the syntax dockerfile won't be needed (see the second link, in the env section).

I need to go, without finishing my message, but it's a good start

@codecov-commenter
Copy link

Codecov Report

Merging #2314 (788a70f) into main (b0bb50f) will increase coverage by 0.02%.
The diff coverage is n/a.

📣 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     
Impacted Files Coverage Δ
megalinter/reporters/UpdatedSourcesReporter.py 89.74% <0.00%> (+2.56%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nvuillam
Copy link
Member

nvuillam commented Feb 6, 2023

🦙 MegaLinter status: ⚠️ WARNING

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
⚠️ MARKDOWN 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
⚠️ PYTHON 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
⚠️ PYTHON pyright 176 274 17.19s
✅ REPOSITORY checkov yes no 27.53s
⚠️ REPOSITORY 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.

MegaLinter is graciously provided by OX Security

@echoix
Copy link
Collaborator

echoix commented Feb 6, 2023

@bdovaz
Copy link
Collaborator Author

bdovaz commented Feb 6, 2023

@echoix about the variable name, it is not a mistake, it is called that way in PHIVE:

https://github.com/phar-io/phive/blob/c5985a10f95be9a21b79e7ee19bef39dc85e9f95/src/shared/config/EnvironmentAuthConfig.php#L18

And about the rest, you are right I understand that it would serve with a:

RUN export GITHUB_AUTH_TOKEN="$(cat /run/secrets/GITHUB_TOKEN)" && phive

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 upload-docker.sh and use the native actions for Docker.

@nvuillam
Copy link
Member

nvuillam commented Feb 6, 2023

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 ^^

@echoix
Copy link
Collaborator

echoix commented Feb 6, 2023

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

@echoix
Copy link
Collaborator

echoix commented Feb 6, 2023

@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.

@bdovaz bdovaz force-pushed the dev/set-github-token-in-phive branch from 7f6f0ef to 7bd3c3a Compare February 11, 2023 11:08
@bdovaz bdovaz enabled auto-merge (squash) February 11, 2023 11:38
@nvuillam nvuillam merged commit d850e81 into main Feb 11, 2023
@nvuillam nvuillam deleted the dev/set-github-token-in-phive branch February 11, 2023 19:32
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.

4 participants