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

Adds support for SA annotations #167

Merged
merged 1 commit into from
Feb 8, 2021
Merged

Adds support for SA annotations #167

merged 1 commit into from
Feb 8, 2021

Conversation

kkapoor1987
Copy link
Contributor

@kkapoor1987 kkapoor1987 commented Jan 18, 2021 •
edited
Loading

Solves #160, #139

Signed-off-by: Karan Kapoor karan.kapoor@pega.com

@vmwclabot
Copy link
Member

@kkapoor1987, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

@kkapoor1987
Copy link
Contributor Author

@dimalbaby Can you please review this !!!

@kkapoor1987 kkapoor1987 force-pushed the feature/annotationforSA branch 3 times, most recently from 8d69f58 to 5d8c783 Compare January 22, 2021 19:20
@kkapoor1987
Copy link
Contributor Author

@viveksyngh Can you review and help me merge these changes

@viveksyngh
Copy link
Member

@kkapoor1987 could you please rebase and update your branch with master ?

@kkapoor1987
Copy link
Contributor Author

kkapoor1987 commented Feb 1, 2021 •
edited
Loading

@viveksyngh thanks for the update... I believe it's ready for review now
Hi @jeremyrickard @viveksyngh can you provide feedback on the PR

viveksyngh
viveksyngh approved these changes Feb 3, 2021
View reviewed changes
Copy link
Member

@viveksyngh viveksyngh left a comment

Choose a reason for hiding this comment

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

LGTM

Cryptophobia
Cryptophobia suggested changes Feb 6, 2021
View reviewed changes
Copy link
Contributor

@Cryptophobia Cryptophobia left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@kkapoor1987 , thank you for the contribution!

Do we need to update the chart version here?

Just increment here by the minor version for some of users who could be using helm charts servers and helm packaging and versioning the log-router chart:
https://github.com/vmware/kube-fluentd-operator/blob/master/charts/log-router/Chart.yaml#L7

@kkapoor1987
Adds support for SA annotations
f21f079 
Adds support for fullName override to the chart
Solves vmware#160

Signed-off-by: Karan Kapoor <karan.kapoor@pega.com>
@kkapoor1987
Copy link
Contributor Author

@Cryptophobia Thanks for that ...I just pushed it.

Cryptophobia
Cryptophobia approved these changes Feb 8, 2021
View reviewed changes
Copy link
Contributor

@Cryptophobia Cryptophobia left a comment

Choose a reason for hiding this comment

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

lgtm

@Cryptophobia Cryptophobia merged commit 9bf8299 into vmware:master Feb 8, 2021
@Cryptophobia Cryptophobia mentioned this pull request Feb 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viveksyngh viveksyngh viveksyngh approved these changes

@Cryptophobia Cryptophobia Cryptophobia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kkapoor1987 @vmwclabot @viveksyngh @Cryptophobia