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

Create top level eni block for Helm values and add more options to it #14470

Merged
merged 3 commits into from
Dec 28, 2020
Merged

Create top level eni block for Helm values and add more options to it #14470

merged 3 commits into from
Dec 28, 2020

Conversation

ungureanuvladvictor
Copy link
Member

@ungureanuvladvictor ungureanuvladvictor commented Dec 21, 2020 •
edited

Best to review commit by commit.

1st one adds the eni top level helm block and put the enabled option under it.
2nd commit adds the cilium-operator-aws specific configs flags to helm and defaults them to what the operator currently has (details in the commit message).

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 21, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Dec 21, 2020
@ungureanuvladvictor ungureanuvladvictor added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Dec 21, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 21, 2020
@ungureanuvladvictor ungureanuvladvictor force-pushed the vladu/eni-helm branch 5 times, most recently from 222b079 to 574c8d2 Compare December 21, 2020 21:46
@ungureanuvladvictor ungureanuvladvictor marked this pull request as ready for review December 21, 2020 21:49
@ungureanuvladvictor
Copy link
Member Author

test-me-please

aanm
aanm requested changes Dec 22, 2020
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

The changes LGTM. The only changes we need is to write some upgrade notes because we are changing the behavior of .Values.eni to .Values.eni.enabled

kkourt
kkourt approved these changes Dec 22, 2020
View reviewed changes
Copy link
Contributor

@kkourt kkourt left a comment •
edited

Choose a reason for hiding this comment

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

Other than Andre's comment, looks good to me!

I did a few greps to see if there are still uses of the old eni parameter, and there do not seem to be any 👍

Both:

$ git grep 'Values.eni[^\.]'
$ git grep -- '--set eni='

return no results.

@ungureanuvladvictor
Copy link
Member Author

ungureanuvladvictor commented Dec 22, 2020 •
edited

test-me-please

@aanm updated with a small line in the upgrade guide, let me know if you want more details there.

christarazi
christarazi approved these changes Dec 22, 2020
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM. Small nit on the Helm value names

install/kubernetes/cilium/README.md Outdated Show resolved Hide resolved
@ungureanuvladvictor
Copy link
Member Author

test-gke

@ungureanuvladvictor
Create top level eni block for helm values
9cd221f 
Signed-off-by: Vlad Ungureanu <vladu@palantir.com>
@ungureanuvladvictor
Add more ENI options to the Helm chart
74aa2ce 
Added the following entries which map to the AWS operator config options:
- updateEC2AdapterLimitViaAPI -> update-ec2-adapter-limit-via-api
- awsReleaseExcessIPs -> aws-release-excess-ips
- EC2APIEndpoint -> ec2-api-endpoint
- eniTags -> eni-tags
- subnetIDsFilter -> subnet-ids-filter
- subnetTagsFilter -> subnet-tags-filter

Signed-off-by: Vlad Ungureanu <vladu@palantir.com>
@ungureanuvladvictor
Add upgrade docs
fd36cfc 
Signed-off-by: Vlad Ungureanu <vladu@palantir.com>
@ungureanuvladvictor
Copy link
Member Author

test-me-please

@aanm aanm merged commit 12b7687 into master Dec 28, 2020
@aanm aanm deleted the vladu/eni-helm branch December 28, 2020 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi christarazi approved these changes

@kkourt kkourt kkourt approved these changes

@aanm aanm aanm approved these changes

@tgraf tgraf Awaiting requested review from tgraf

@joestringer joestringer Awaiting requested review from joestringer

Assignees

@kkourt kkourt

@joestringer joestringer

@tgraf tgraf

@aanm aanm

Labels
release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ungureanuvladvictor @kkourt @christarazi @aanm @joestringer @tgraf