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 Dockerfile sso #3307

Merged
merged 1 commit into from
Apr 11, 2022
Merged

Conversation

Kevan-Y
Copy link
Contributor

@Kevan-Y Kevan-Y commented Mar 24, 2022

Issue This PR Addresses

Fixes #1668

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

  • Add multi-staging layers
  • Add Docker Healthcheck
  • Use a smaller base image

Image is a bit bigger because we install the pnpm package

❯ docker images | grep refactor/sso
refactor/sso                                    new                d31f703b6c2b   18 seconds ago       132MB
refactor/sso                                    old                02baf2192da0   2 weeks ago          135MB

Steps to test the PR

  • pnpm services:start
  • Go to http://localhost/v1/auth/healthcheck

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Mar 24, 2022

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Let's drop the dev/dependencies stuff, we don't need.

src/api/sso/Dockerfile Outdated Show resolved Hide resolved
@AmasiaNalbandian AmasiaNalbandian requested review from AmasiaNalbandian and a team March 24, 2022 13:31
@AmasiaNalbandian AmasiaNalbandian added this to the 2.9 Release milestone Mar 24, 2022
@vercel
Copy link

vercel bot commented Mar 24, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/humphd/telescope/A9yaRg6rL2jAqyB7mrsLvsq5wZUw
✅ Preview: Failed

[Deployment for 6adaa55 failed]

humphd
humphd previously approved these changes Mar 24, 2022
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Looks good

@Kevan-Y
Copy link
Contributor Author

Kevan-Y commented Mar 24, 2022

Great, I'm gonna squash those commits.

humphd
humphd previously approved these changes Apr 11, 2022
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Great work not giving up on this.

@humphd humphd added type: test Creation and development of test dependencies Pull requests that update a dependency file labels Apr 11, 2022
Update playwright to 1.20.2
@Kevan-Y
Copy link
Contributor Author

Kevan-Y commented Apr 11, 2022

@humphd, sorry to make you re-review again, this is the right change with pnpm + adding Docker best practices

@Kevan-Y Kevan-Y marked this pull request as ready for review April 11, 2022 20:01
@humphd
Copy link
Contributor

humphd commented Apr 11, 2022

This i ready to go, just needs one more r+

@humphd humphd merged commit cdc945e into Seneca-CDOT:master Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: docker dependencies Pull requests that update a dependency file type: test Creation and development of test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply Dockerfile best practices from Snyk recommendations
5 participants