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 calico/node makefile to default variables properly #979

Merged
merged 1 commit into from
Aug 1, 2017

Conversation

lwr20
Copy link
Member

@lwr20 lwr20 commented Jul 31, 2017

Description

A lot of variables are set using :=, but I think the intent of most of those is to provide a default setting if the user hasn't overridden the variable themselves. This PR changes all the settings which a user might want to override to ?= which should give the desired behaviour.

Release Note

None required

@lwr20 lwr20 force-pushed the lwr-default-fix branch from 89f8174 to c273bdc Compare July 31, 2017 13:53
@lwr20 lwr20 requested a review from robbrockbank July 31, 2017 18:41
@lwr20 lwr20 self-assigned this Jul 31, 2017
@lwr20 lwr20 requested review from ozdanborne and removed request for robbrockbank August 1, 2017 10:08
@tmjd
Copy link
Member

tmjd commented Aug 1, 2017

Feels a little extreme that we would need the ability to set all those but I don't think it harms anything (though I'm not a make expert).
LGTM

@ozdanborne
Copy link
Member

I'm a little concerned that the scope of this PR means that we're now pushing people away from versions.yml and towards environment variables. However, I do see the need for environment variables as a way to more easily integrate with automated systems (jenkins). So LGTM.

Maybe in the future we'll have better yaml support and it will make sense to revert this. Who knows

@ozdanborne ozdanborne merged commit 979ce82 into projectcalico:master Aug 1, 2017
@lwr20 lwr20 deleted the lwr-default-fix branch July 28, 2021 19:30
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.

3 participants