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

refactor: use = with ENV in dockerfiles #6196

Merged

Conversation

PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

see: https://docs.docker.com/go/dockerfile/rule/legacy-key-value-format/

Basically avoid old format of ENV in docker files

What was done?

see commit

How Has This Been Tested?

Building the containers locally, seems all fine

Breaking Changes

Please describe any breaking changes your code introduces

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@PastaPastaPasta PastaPastaPasta added this to the 22 milestone Aug 11, 2024
@@ -15,12 +15,12 @@ ARG guix_checksum_aarch64=72d807392889919940b7ec9632c45a259555e6b0942ea7bfd13110
ARG guix_checksum_x86_64=236ca7c9c5958b1f396c2924fcc5bc9d6fdebcb1b4cf3c7c6d46d4bf660ed9c9
ARG builder_count=32

ENV PATH /root/.config/guix/current/bin:$PATH
ENV PATH=/root/.config/guix/current/bin:$PATH
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's add "" also?

Suggested change
ENV PATH=/root/.config/guix/current/bin:$PATH
ENV PATH="/root/.config/guix/current/bin:$PATH"

because previous PATH can have not only alpha-numeric characters

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense to me

ogabrielides
ogabrielides previously approved these changes Aug 12, 2024
Copy link
Collaborator

@ogabrielides ogabrielides left a comment

Choose a reason for hiding this comment

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

utACK

@@ -5,11 +5,11 @@ LABEL description="Dockerised DashCore, built from CI"
ARG USER_ID
ARG GROUP_ID

ENV HOME=/home/dash
ENV HOME="/home/dash"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this particular case no really need quotes :D

Copy link
Member Author

Choose a reason for hiding this comment

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

well you told me to add quotes!

Copy link
Collaborator

Choose a reason for hiding this comment

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

do it wisely!

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

utACK 92f82f6

Copy link
Collaborator

@ogabrielides ogabrielides left a comment

Choose a reason for hiding this comment

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

utACK

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK 92f82f6

@PastaPastaPasta PastaPastaPasta merged commit bc73522 into dashpay:develop Aug 12, 2024
16 checks passed
@PastaPastaPasta PastaPastaPasta deleted the refactor-docker-warnings branch August 12, 2024 10:17
@PastaPastaPasta PastaPastaPasta modified the milestones: 22, 21.2 Aug 12, 2024
@UdjinM6 UdjinM6 modified the milestones: 21.2, 22 Oct 29, 2024
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