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

chore: add directories to dockerignore (used to speed up local dev) #12620

Merged

Conversation

blakepettersson
Copy link
Member

@blakepettersson blakepettersson commented Feb 25, 2023

Add a bunch of directories to .dockerignore which are not required when doing the initial build of the Argo images. The background is that when developing locally, I use Skaffold to incrementally rebuild the image on any changes (see #11980). When doing any changes to directories which do not strictly require a rebuild of the Docker image, Skaffold proceeds to rebuild the image. This PR just adds the directories which should not force a rebuild of the image.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I have signed off all my commits as required by DCO
  • My build is green (troubleshooting builds).

Add a bunch of directories to `.dockerignore` which are not required
within the production image.

Signed-off-by: Blake Pettersson <[email protected]>
@codecov
Copy link

codecov bot commented Feb 25, 2023

Codecov Report

Base: 47.78% // Head: 47.78% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (c6f32e5) compared to base (743b09f).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #12620   +/-   ##
=======================================
  Coverage   47.78%   47.78%           
=======================================
  Files         246      246           
  Lines       41921    41944   +23     
=======================================
+ Hits        20031    20045   +14     
- Misses      19891    19898    +7     
- Partials     1999     2001    +2     
Impacted Files Coverage Δ
controller/health.go 78.00% <0.00%> (-10.38%) ⬇️
applicationset/generators/merge.go 53.24% <0.00%> (-4.37%) ⬇️
pkg/apis/application/v1alpha1/types.go 58.54% <0.00%> (-0.49%) ⬇️
applicationset/generators/cluster.go 80.27% <0.00%> (ø)
controller/appcontroller.go 53.04% <0.00%> (+0.41%) ⬆️
applicationset/generators/matrix.go 77.50% <0.00%> (+7.75%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@34fathombelow
Copy link
Member

I do not see any of these directories in our final images or any COPY or ADD instructions in the Dockerfile. I'm I missing something?

@blakepettersson blakepettersson changed the title chore: slim down production image chore: add directories to dockerignore (used to speed up local dev) Feb 25, 2023
@blakepettersson
Copy link
Member Author

@34fathombelow sorry that was sloppy phrasing from my side. What I meant to say was that when building the image locally, changes to any files that are within the directories that are now in .dockerignore forces a rebuild of the Docker image. Since those files shouldn't force a rebuild, I added those to .dockerignore.

To be clear this is an issue only for local development (see description for more context) 😄

Copy link
Member

@34fathombelow 34fathombelow left a comment

Choose a reason for hiding this comment

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

lgtm

@crenshaw-dev crenshaw-dev merged commit 7b7c5ae into argoproj:master Feb 25, 2023
@WitoDelnat
Copy link
Contributor

Hi! I was trying to run Argo CD locally for development and I think this PR has impacted the build on master.

I'm unfamiliar with the build setup but running make start fails. It tries to build an image from the Dockerfile in test/container/dockerfile which relies on files that are missing because test/ was added to the dockerignore in this PR.

Let me know if I can help.

test/container/dockerfile:64
COPY ./test/fixture/testrepos/id_rsa.pub /root/.ssh/authorized_keys
etc

@blakepettersson
Copy link
Member Author

@WitoDelnat sorry about that! If you add !test/fixture to .dockerignore does that solve your issue?

blakepettersson added a commit to blakepettersson/argo-cd that referenced this pull request Feb 27, 2023
argoproj#12620 introduced the ignore of the `test/` directory, which had the
unintended effect of breaking `make start`. To amend that, add an
override for `test/fixture` in order to unbreak
`test/container/dockerfile`.

Signed-off-by: Blake Pettersson <[email protected]>
@blakepettersson
Copy link
Member Author

If so #12640 will fix it for everyone else that might be affected

@WitoDelnat
Copy link
Contributor

WitoDelnat commented Feb 27, 2023

No worries! It was easy to work around. You missed a file to exclude (see PR) but after that it should be good to go!

crenshaw-dev added a commit that referenced this pull request Mar 2, 2023
* fix: exclude test/fixtures from .dockerignore

#12620 introduced the ignore of the `test/` directory, which had the
unintended effect of breaking `make start`. To amend that, add an
override for `test/fixture` in order to unbreak
`test/container/dockerfile`.

Signed-off-by: Blake Pettersson <[email protected]>

* fix: also exclude `test/containers`

Signed-off-by: Blake Pettersson <[email protected]>

---------

Signed-off-by: Blake Pettersson <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]>
rumstead pushed a commit to rumstead/argo-cd that referenced this pull request Mar 3, 2023
* fix: exclude test/fixtures from .dockerignore

argoproj#12620 introduced the ignore of the `test/` directory, which had the
unintended effect of breaking `make start`. To amend that, add an
override for `test/fixture` in order to unbreak
`test/container/dockerfile`.

Signed-off-by: Blake Pettersson <[email protected]>

* fix: also exclude `test/containers`

Signed-off-by: Blake Pettersson <[email protected]>

---------

Signed-off-by: Blake Pettersson <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]>
Signed-off-by: rumstead <[email protected]>
blakepettersson added a commit to blakepettersson/argo-cd that referenced this pull request Jun 18, 2023
Add the ability to specify `GIT_TAG`, `GIT_COMMIT`, `BUILD_DATE` and
`GIT_TREE_STATE` as optional build-args. As well as resolving argoproj#13683
(which was caused by argoproj#12620), this has the bonus of making the
`docker build` slightly more deterministic (since we now have the
ability to specify the same inputs into the docker build which was
hitherto computed on every `docker build`).

Signed-off-by: Blake Pettersson <[email protected]>
crenshaw-dev pushed a commit that referenced this pull request Jun 20, 2023
* build: add build-args for git-commit etc

Add the ability to specify `GIT_TAG`, `GIT_COMMIT`, `BUILD_DATE` and
`GIT_TREE_STATE` as optional build-args. As well as resolving #13683
(which was caused by #12620), this has the bonus of making the
`docker build` slightly more deterministic (since we now have the
ability to specify the same inputs into the docker build which was
hitherto computed on every `docker build`).

Signed-off-by: Blake Pettersson <[email protected]>

* Update .github/workflows/image-reuse.yaml

Co-authored-by: Josh Soref <[email protected]>
Signed-off-by: Blake Pettersson <[email protected]>

---------

Signed-off-by: Blake Pettersson <[email protected]>
Co-authored-by: Josh Soref <[email protected]>
yyzxw pushed a commit to yyzxw/argo-cd that referenced this pull request Aug 9, 2023
Add a bunch of directories to `.dockerignore` which are not required
within the production image.

Signed-off-by: Blake Pettersson <[email protected]>
yyzxw pushed a commit to yyzxw/argo-cd that referenced this pull request Aug 9, 2023
* fix: exclude test/fixtures from .dockerignore

argoproj#12620 introduced the ignore of the `test/` directory, which had the
unintended effect of breaking `make start`. To amend that, add an
override for `test/fixture` in order to unbreak
`test/container/dockerfile`.

Signed-off-by: Blake Pettersson <[email protected]>

* fix: also exclude `test/containers`

Signed-off-by: Blake Pettersson <[email protected]>

---------

Signed-off-by: Blake Pettersson <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]>
yyzxw pushed a commit to yyzxw/argo-cd that referenced this pull request Aug 9, 2023
* build: add build-args for git-commit etc

Add the ability to specify `GIT_TAG`, `GIT_COMMIT`, `BUILD_DATE` and
`GIT_TREE_STATE` as optional build-args. As well as resolving argoproj#13683
(which was caused by argoproj#12620), this has the bonus of making the
`docker build` slightly more deterministic (since we now have the
ability to specify the same inputs into the docker build which was
hitherto computed on every `docker build`).

Signed-off-by: Blake Pettersson <[email protected]>

* Update .github/workflows/image-reuse.yaml

Co-authored-by: Josh Soref <[email protected]>
Signed-off-by: Blake Pettersson <[email protected]>

---------

Signed-off-by: Blake Pettersson <[email protected]>
Co-authored-by: Josh Soref <[email protected]>
tesla59 pushed a commit to tesla59/argo-cd that referenced this pull request Dec 16, 2023
* build: add build-args for git-commit etc

Add the ability to specify `GIT_TAG`, `GIT_COMMIT`, `BUILD_DATE` and
`GIT_TREE_STATE` as optional build-args. As well as resolving argoproj#13683
(which was caused by argoproj#12620), this has the bonus of making the
`docker build` slightly more deterministic (since we now have the
ability to specify the same inputs into the docker build which was
hitherto computed on every `docker build`).

Signed-off-by: Blake Pettersson <[email protected]>

* Update .github/workflows/image-reuse.yaml

Co-authored-by: Josh Soref <[email protected]>
Signed-off-by: Blake Pettersson <[email protected]>

---------

Signed-off-by: Blake Pettersson <[email protected]>
Co-authored-by: Josh Soref <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants