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 openFaasImagePullPolicy for faas-netes container (and not faasnet… #380

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@swachter
Copy link

swachter commented Mar 5, 2019

Description

The faas-netes container uses the wrong imagePullPolicy it should use openFaasImagePullPolicy and not faasnetesd.imagePullPolicy

Motivation and Context

  • I have raised an issue to propose this change (required)

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@derek

This comment has been minimized.

Copy link

derek bot commented Mar 5, 2019

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --sign-off and then git push --force.

@alexellis
Copy link
Member

alexellis left a comment

This doesn't look right.

useswachte
use openFaasImagePullPolicy for faas-netes container (and not faasnet…
…esd.imagePullPolicy)

Signed-off-by: useswachte <stefan.wachter.extern@usu.de>

@swachter swachter force-pushed the swachter:imagePullPolicy-of-faas-netes branch from e90c762 to ffdc6aa Mar 6, 2019

@derek derek bot removed the no-dco label Mar 6, 2019

@swachter

This comment has been minimized.

Copy link
Author

swachter commented Mar 6, 2019

This doesn't look right.

From the documentation of the helm chart:

Parameter Description Default
faasnetesd.imagePullPolicy Image pull policy for deployed functions Always
openfaasImagePullPolicy Image pull policy for openfaas components, can change to IfNotPresent in offline env Always

The gateway pod with its gateway and faas-netes containers are a openfaas component, i.e. their image pull policy should be set by openfaasImagePullPolicy. On the other hand the faasnetesd.imagePullPolicy setting is used to set the environment variable image_pull_policy that in turn is considered by the two mentioned containers.

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 6, 2019

You're right, but perhaps the issue here is that the naming is really confusing. Can we do better? @LucasRoesler @stefanprodan

Btw you have a rebase conflict on this PR.

Alex

@derek derek bot added the no-dco label Mar 6, 2019

@derek

This comment has been minimized.

Copy link

derek bot commented Mar 6, 2019

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --sign-off and then git push --force.

useswachte
merged master
Signed-off-by: useswachte <stefan.wachter.extern@usu.de>

@swachter swachter force-pushed the swachter:imagePullPolicy-of-faas-netes branch from 4b4c56f to befca01 Mar 6, 2019

@derek derek bot removed the no-dco label Mar 6, 2019

@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Mar 6, 2019

@alexellis perhaps the clearest names would be functionPullPolicy and componentPullPolicy

I also think the variable name faasnetesd.imagePullPolicy does the opposite of what I would expect. I would expect (without reading the docs) that it defines the pull policy for the component, not for functions. The function pull policy should be the un-scoped value.

I don't think there is a non-breaking way to fix this though. of course, i know of high profile projects that have broken their helm charts (like nginx-ingress). so as long as we change the version appropriately and signal the change, we should be ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.