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

Use latest compose specs #5854

Closed

Conversation

lucasfcnunes
Copy link
Member

@lucasfcnunes lucasfcnunes commented Nov 11, 2022

What type of PR is this?

  • Refactor

Description

Commits to improve/refactor/update the Redash project to current docker specs.

How is this tested?

  • Manually

make build runs as always.

Related Tickets & Documents

@lucasfcnunes
Copy link
Member Author

Build failing because of CircleCI's docker [compose] version (v1)?

Building server
Sending build context to Docker daemon  14.15MB
Error response from daemon: Dockerfile parse error line 12: Unknown flag: mount
ERROR: Service 'server' failed to build : Build failed

@guidopetri
Copy link
Contributor

Hi @lucasfcnunes , thanks for your contribution! We've updated a lot of things now that we're community-driven so would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

Additionally, if you could split up the build caching changes into a separate PR, that would be ideal - that way we can keep the required changes to comply with recommendations separate from complex Dockerfile changes.

Dockerfile Outdated
RUN npm install --global --force [email protected]
RUN \
--mount=type=cache,target=/root/.npm \
npm install --global --force [email protected]
Copy link
Member

Choose a reason for hiding this comment

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

As a data point, we've updated to [email protected] a few weeks ago (241dcfa), so probably best to use that version here too.

@justinclift
Copy link
Member

Looking over this PR, it's probably going to need very manual re-creation as we've been changing so many things in the Dockerfile over the last few weeks.

Not impossible in any way, but likely just pretty time consuming.

@guidopetri
Copy link
Contributor

Agreed, that's why I think it's better splitting up into more "style" and more "feature" type changes.

@justinclift
Copy link
Member

Good thinking. 😄

@lucasfcnunes lucasfcnunes force-pushed the docker-compose-v2 branch 3 times, most recently from b65819c to d3f1236 Compare February 4, 2024 11:53
Signed-off-by: Lucas Fernando Cardoso Nunes <[email protected]>
@lucasfcnunes lucasfcnunes changed the title Update to latest docker specs Use latest compose specs Feb 4, 2024
@lucasfcnunes lucasfcnunes marked this pull request as ready for review February 4, 2024 11:56
Copy link

codecov bot commented Feb 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (49a5e74) 63.37% compared to head (b65819c) 63.37%.

❗ Current head b65819c differs from pull request most recent head 84752b4. Consider uploading reports for the commit 84752b4 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5854   +/-   ##
=======================================
  Coverage   63.37%   63.37%           
=======================================
  Files         162      162           
  Lines       13170    13170           
  Branches     1819     1819           
=======================================
  Hits         8347     8347           
  Misses       4532     4532           
  Partials      291      291           

@guidopetri
Copy link
Contributor

@lucasfcnunes thanks for updating! I'm not a huge fan personally of changing the docker compose filename, but I see it's the preferred standard now (why change what isn't broken?). It seems we need to update something else on the CI if we want to rename the files though, otherwise the CI won't pass.

@lucasfcnunes
Copy link
Member Author

@lucasfcnunes thanks for updating! I'm not a huge fan personally of changing the docker compose filename, but I see it's the preferred standard now (why change what isn't broken?). It seems we need to update something else on the CI if we want to rename the files though, otherwise the CI won't pass.

@guidopetri I think it's expected as it's using the ci.yml from master branch, not sure.
*These lines prevent this.

image

@guidopetri
Copy link
Contributor

Ah, I see, that's new to me! Thanks for explaining. Let's go ahead and merge then, and hopefully it passes in master. Thanks again for your contribution!

myonlylonely pushed a commit to myonlylonely/redash-custom that referenced this pull request Feb 4, 2024
@lucasfcnunes
Copy link
Member Author

Seems to be working as expected https://github.com/getredash/redash/actions/runs/7777381717/workflow#L33

What I don't understand is how this PR was "merged" into master but is still open...
image

@guidopetri
Copy link
Contributor

Me neither, I think it was a github bug. I'm going to close it, but thanks again!

@guidopetri guidopetri closed this Feb 5, 2024
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants