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.

Sign up for GitHub

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

Jump to bottom

Build multi-platform docker image on every PR #379

Merged
merged 1 commit into from
Dec 2, 2024
Merged

Build multi-platform docker image on every PR #379

merged 1 commit into from
Dec 2, 2024

Conversation

runejo
Copy link
Contributor

@runejo runejo commented Dec 2, 2024

No description provided.

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Dec 2, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

likp
likp approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@likp likp left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM , but the CI build is failing

@runejo runejo merged commit 4417341 into main Dec 2, 2024
8 of 9 checks passed
@runejo runejo deleted the docker-ci-build branch December 2, 2024 13:30
michaelpande
michaelpande reviewed Dec 2, 2024
View reviewed changes
Dockerfile

# production environment
FROM nginxinc/nginx-unprivileged:1.27.2-alpine3.20
COPY --from=build /packages/pxweb2/dist /usr/share/nginx/html
COPY --from=build /app/packages/pxweb2/dist /usr/share/nginx/html
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be wrong, is the build step building to the wrong location perhaps? It should not be necessary to add /app/ here 🤔

Copy link
Contributor Author

@runejo runejo Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It was failing on COPY, but it could also be fixed like this, without leading slash
COPY --from=build packages/pxweb2/dist /usr/share/nginx/html

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be correct the code is placed in /app.
When is test localy the build does not fail.

@runejo
Copy link
Contributor Author

runejo commented Dec 2, 2024

LGTM , but the CI build is failing

Yes, it was why I made this PR
main branch is now failing on arm64 cpu arch, because of problems in package-lock.json

@runejo runejo mentioned this pull request Dec 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@likp likp likp approved these changes

@michaelpande michaelpande michaelpande left review comments

@MikaelNordberg MikaelNordberg Awaiting requested review from MikaelNordberg MikaelNordberg is a code owner

@KentMossback KentMossback Awaiting requested review from KentMossback KentMossback is a code owner

@PerIngeVaaje PerIngeVaaje Awaiting requested review from PerIngeVaaje PerIngeVaaje is a code owner

@JohannesFinsveen JohannesFinsveen Awaiting requested review from JohannesFinsveen

@oyessb oyessb Awaiting requested review from oyessb oyessb is a code owner

@SjurSutterudSagen SjurSutterudSagen Awaiting requested review from SjurSutterudSagen SjurSutterudSagen is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@runejo @michaelpande @likp