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

fix(nginx archive recipe): Fixes to various configuration files. #3624

Merged
merged 3 commits into from
Sep 5, 2023

Conversation

jbocce
Copy link
Collaborator

@jbocce jbocce commented Aug 31, 2023

Context

Fixes #3423

Note that the OpenResty-Orthanc-Keycloak recipe is still broken, but some changes were made for it.

Changes & Results

Various changes to docker, nginx and OHIF configuration files.

Testing

Follow the nginx-image-archive recipe in the OHIF docs.

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • OS: Windows 11
  • Node version: 16.14.0
  • Browser: Chrome 116.0.5845.111

@netlify
Copy link

netlify bot commented Aug 31, 2023

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 7ff482e
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/64f1f200b7973f00084e09b4
😎 Deploy Preview https://deploy-preview-3624--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Aug 31, 2023

Deploy Preview for ohif-platform-docs canceled.

Name Link
🔨 Latest commit 7ff482e
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/64f1f20087a0550008a5c1cf

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #3624 (7ff482e) into master (b170e7f) will not change coverage.
Report is 1 commits behind head on master.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3624   +/-   ##
=======================================
  Coverage   42.54%   42.54%           
=======================================
  Files          80       80           
  Lines        1462     1462           
  Branches      340      340           
=======================================
  Hits          622      622           
  Misses        675      675           
  Partials      165      165           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01500a2...7ff482e. Read the comment docs.

@jbocce jbocce marked this pull request as ready for review August 31, 2023 02:38
@jbocce jbocce requested a review from sedghi August 31, 2023 02:39
Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

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

Looks great, fixed it for ARM


RUN mkdir /usr/src/app
WORKDIR /usr/src/app

ENV APP_CONFIG=config/docker_openresty-orthanc-keycloak.js
ENV PATH /usr/src/app/node_modules/.bin:$PATH

# Copy Files
COPY .docker /usr/src/app/.docker
Copy link
Member

Choose a reason for hiding this comment

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

is there any folder that we don't need to copy? maybe we say copy all excluding this and that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the list to exclude is bigger than the list to include.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, I couldn't find a way to exclude files using the COPY command. But... I think I may have found a way using the .dockerignore file. Stay tuned.

@jbocce jbocce requested a review from sedghi September 1, 2023 14:17
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.

[Bug] building Nginx + Image Archive recipe fails
2 participants