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

add custom useragent suffix #910

Merged
merged 1 commit into from
Jun 2, 2021
Merged

Conversation

vdhanan
Copy link
Contributor

@vdhanan vdhanan commented May 27, 2021

Is this a bug fix or adding new feature?
new feature

What is this PR about? / Why do we need it?
modifies the aws sdk client's useragent string to differentiate the ebs csi driver's requests

What testing is done?
turned on debug logs and confirmed that user agent had new string appended

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 27, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdhanan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from gnufied and wongma7 May 27, 2021 18:09
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 27, 2021
@coveralls
Copy link

coveralls commented May 27, 2021

Pull Request Test Coverage Report for Build 2022

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.06%) to 79.062%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/cloud/cloud.go 0 2 0.0%
Totals Coverage Status
Change from base Build 2021: -0.06%
Covered Lines: 1990
Relevant Lines: 2517

💛 - Coveralls

pkg/cloud/cloud.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 28, 2021
@vdhanan vdhanan force-pushed the useragent branch 3 times, most recently from 940d4ec to a17ddc8 Compare June 1, 2021 17:08
@@ -39,6 +39,8 @@ spec:
- --logtostderr
- --v=2
env:
- name: CSI_DRIVER_NAME
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we want to expose the name as env variable? Feel like it would be more accurate if the useragent is fixed..

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I agree, better to just keep it fixed/hardcoded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my thinking was that if the driver's name changed for whatever reason, we'd only have to change it in one place. i don't have a strong preference here, so i'll just hardcode it.

@@ -2,7 +2,7 @@ apiVersion: v1
appVersion: "1.0.0"
name: aws-ebs-csi-driver
description: A Helm chart for AWS EBS CSI Driver
version: 1.2.0
version: 1.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

correct me if I'm wrong.. it should be 1.0.3 instead of 1.3.0 as the driver version is still on 1.0.x

Copy link
Contributor

Choose a reason for hiding this comment

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

I think, if we avoid exposing env variable, then we dont need to change the chart at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the helm chart version is 1.2.0 on master right now. is that a problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, the helm chart version doesn't have to and likely won't match the version of the application. That is what appVersion is for to specify the version of the application to use if the user doesn't override it. If the chart keeps getting new features added that work with 1.0.0. of the driver, it will continue to increase in version independent of the driver version.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 2, 2021
@wongma7
Copy link
Contributor

wongma7 commented Jun 2, 2021

/lgtm

thanks

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2021
@wongma7 wongma7 merged commit 2b61230 into kubernetes-sigs:master Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants