- 
                Notifications
    You must be signed in to change notification settings 
- Fork 61
Adding a package to contain common sidecar flags #202
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
Conversation
| Hi @DerekFrank. Thanks for your PR. I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with  Once the patch is verified, the new status will be reflected by the  I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a link to the conversation thread in Slack in the PR description.
Thanks for considering the monorepo sidecar design as an example on how to extract the flags! Hey Ge, could you please check that these params match your analysis in https://docs.google.com/document/d/1AKqJeAlBL8PkH8D9zABCZ82Bk1N46EygKPvVh5p4-qU/edit? cc @ConnorJC3.
/cc @mowangdk
/ok-to-test
9ed3658    to
    43a8768      
    Compare
  
            
          
                config/config.go
              
                Outdated
          
        
      |  | ||
| logger := klog.Background() | ||
|  | ||
| // Prepare http endpoint for metrics + leader election healthz | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm honestly not sure if we want to move this into this function as well, I did it because we need access to mux for le.PrepareHealthCheck(mux, leaderelection.DefaultHealthCheckTimeout) and as far as I can tell nothing else in the sidecar's main.go files reference the mux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it looks odd here. Get the mux from the callers, as an argument.
43a8768    to
    4814a37      
    Compare
  
            
          
                config/config.go
              
                Outdated
          
        
      |  | ||
| logger := klog.Background() | ||
|  | ||
| // Prepare http endpoint for metrics + leader election healthz | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it looks odd here. Get the mux from the callers, as an argument.
f2fa3e4    to
    888ae82      
    Compare
  
    888ae82    to
    c772e19      
    Compare
  
    c772e19    to
    f9b84fa      
    Compare
  
    | Made sidecar PRs as well for comparison: 
 The other sidecars don't have leader election, unless I missed one! | 
| /lgtm Thank you for your PR. This will also help the CSI Sidecar AIO project~ | 
| @jsafrane Any other feedback? | 
| /lgtm | 
| [APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DerekFrank, jsafrane 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  | 
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Following a discussion in the #csi channel of the k8s slack, folks generally support moving common sidecar flags into a shared package.
Example sidecar flags
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: