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 smaller base image, based on Alpine #33

Closed
wants to merge 0 commits into from

Conversation

pelithne
Copy link

PR related to #32

The only change is that the base image is changed to a smaller, Alpine based, version. This make the whole app more light weight without changing any features.

The change has been manually tested to work with the current tutorial: https://docs.microsoft.com/en-us/azure/aks/tutorial-kubernetes-prepare-app

However, the docker images commands used in the tutorial will render a slightly different output

@Overv
Copy link

Overv commented Apr 19, 2019

I don't see why this isn't being merged. It reduces the image size from 950 MB to 193 MB!

@Overv
Copy link

Overv commented Apr 23, 2019

@pelithne What do you mean by original repo?

@Overv
Copy link

Overv commented Apr 23, 2019

@neilpeterson @iainfoulds What is stopping you from merging this?

@pelithne
Copy link
Author

@Overv I must have commented in my sleep :-), please disregard!

@iainfoulds
Copy link
Contributor

@Overv I wasn't a repo admin, so didn't receive notifications on this PR. I'm not sure why the original manifest used the larger Ubuntu image. We can prioritize this against other deliverables and review in order to merge it. Please be respectful in how you ask for assistance or updates.

@Overv
Copy link

Overv commented Apr 23, 2019

@iainfoulds I asked this way because I just thought it was strange that such an simple and obvious improvement appeared to be overlooked.

I was going through the tutorial for AKS last week and the first thing I wondered was why such a simple web app required me to push such a huge image. A tutorial ought to promote best practices.

Copy link
Contributor

@zr-msft zr-msft left a comment

Choose a reason for hiding this comment

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

@pelithne thank you so much for this contribution and apologies for the delayed response.

Please take a look at my feedback/questions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file needed to support the base image change? or is it out of scope of this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

many of these changes have been incorporated by other merged PRs. Please rebase.

containers:
- name: azure-vote-front
image: microsoft/azure-vote-front:v1
image: privatepelithne.azurecr.io/azuredocs/azure-vote-front:latest
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 stay a reference to the image in MCR which will appear after a rebase:
https://github.com/Azure-Samples/azure-voting-app-redis/blob/master/azure-vote-all-in-one-redis.yaml#L60

Copy link
Contributor

Choose a reason for hiding this comment

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

Please leave the original values and revert this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

similar to .github/workflows, is this needed to support the proposed image change? or is this out of scope?

@phemmmie
Copy link

new update

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.

5 participants